-
Notifications
You must be signed in to change notification settings - Fork 42
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
feat(examples): add deadline secrets management options to basic example #562
feat(examples): add deadline secrets management options to basic example #562
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, just a few small comments about some wording and formatting.
examples/deadline/All-In-AWS-Infrastructure-Basic/python/README.md
Outdated
Show resolved
Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/python/README.md
Outdated
Show resolved
Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/python/package/config.py
Outdated
Show resolved
Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/python/package/config.py
Outdated
Show resolved
Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/python/package/app.py
Outdated
Show resolved
Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/ts/lib/service-tier.ts
Outdated
Show resolved
Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/ts/lib/service-tier.ts
Outdated
Show resolved
Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/ts/lib/service-tier.ts
Outdated
Show resolved
Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/ts/lib/service-tier.ts
Outdated
Show resolved
Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/ts/lib/service-tier.ts
Outdated
Show resolved
Hide resolved
5ba834b
to
74a1599
Compare
examples/deadline/All-In-AWS-Infrastructure-Basic/python/README.md
Outdated
Show resolved
Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/python/README.md
Outdated
Show resolved
Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/python/package/app.py
Outdated
Show resolved
Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/python/package/lib/service_tier.py
Outdated
Show resolved
Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/ts/lib/service-tier.ts
Outdated
Show resolved
Hide resolved
74a1599
to
8de8e43
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just some minor comments
examples/deadline/All-In-AWS-Infrastructure-Basic/python/README.md
Outdated
Show resolved
Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/python/README.md
Outdated
Show resolved
Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/python/package/config.py
Outdated
Show resolved
Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/python/package/config.py
Outdated
Show resolved
Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/python/package/config.py
Outdated
Show resolved
Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/python/package/lib/service_tier.py
Outdated
Show resolved
Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/ts/bin/app.ts
Outdated
Show resolved
Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/ts/bin/config.ts
Outdated
Show resolved
Hide resolved
Also, can we make the commit message a bit more descriptive? How about something like:
|
8de8e43
to
206d9e4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're going to go to the trouble of changing Secret to Secrets everywhere, we should get these ones as well.
examples/deadline/All-In-AWS-Infrastructure-Basic/python/README.md
Outdated
Show resolved
Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/python/README.md
Outdated
Show resolved
Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/python/package/app.py
Outdated
Show resolved
Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/python/package/app.py
Outdated
Show resolved
Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/python/package/lib/service_tier.py
Outdated
Show resolved
Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/ts/bin/app.ts
Outdated
Show resolved
Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/ts/bin/config.ts
Outdated
Show resolved
Hide resolved
206d9e4
to
5e48505
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only see one last spot we missed changing secret to secrets. It also looks like a few of Jericho's previous comments haven't been addressed.
examples/deadline/All-In-AWS-Infrastructure-Basic/python/package/lib/service_tier.py
Outdated
Show resolved
Hide resolved
46c0e17
to
b39deae
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry to add on one last nitpicky thing at the end of the review, but it seems like it might affect the way the way the readme appears on GitHub.
examples/deadline/All-In-AWS-Infrastructure-Basic/python/README.md
Outdated
Show resolved
Hide resolved
b39deae
to
215e150
Compare
Problem
Even though Secrets Management is on by default, we should update the basic example to have an optional value in the config for the admin credentials and steps in the readme.
Solution
Updated examples with new configurable parameters that allow to disable secret management (that is enabled by default) and provide own admin credentials.
Testing
Deployed updated examples and make sure that secret management is enabled and use provided credentials.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license