Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Packager config mods #338

Merged
merged 3 commits into from
Aug 21, 2019
Merged

Packager config mods #338

merged 3 commits into from
Aug 21, 2019

Conversation

banaag
Copy link
Collaborator

@banaag banaag commented Aug 20, 2019

Changes to amppackager toml file to accommodate ACME config changes.

Addresses #93

@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@banaag banaag requested review from twifkak and Gregable August 20, 2019 20:44
@twifkak twifkak changed the base branch from releases to master August 20, 2019 21:08
@twifkak twifkak removed the cla: no label Aug 20, 2019
@twifkak
Copy link
Member

twifkak commented Aug 20, 2019

@googlebot

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

packager/util/config.go Show resolved Hide resolved
packager/util/config.go Outdated Show resolved Hide resolved
packager/util/config.go Outdated Show resolved Hide resolved
packager/util/config.go Outdated Show resolved Hide resolved
Copy link
Member

@twifkak twifkak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -28,9 +28,13 @@ type Config struct {
Port int
CertFile string // This must be the full certificate chain.
KeyFile string // Just for the first cert, obviously.

// NewCertFile will be read/write. CertFile and NewCertFile will be set when both
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about for the first sentence, "When set, both CertFile and NewCertFile will be read/write." (Though, as you're probably aware, the user-facing documentation is in amppkg.example.toml.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@banaag banaag merged commit ebcc706 into ampproject:master Aug 21, 2019
@banaag banaag deleted the packager-config-mods branch August 21, 2019 22:52
@banaag banaag added the acme All ACME related PRs label Feb 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
acme All ACME related PRs cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants