-
Notifications
You must be signed in to change notification settings - Fork 202
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
Add missing autobuild-disabled flag to publish command #2733
Add missing autobuild-disabled flag to publish command #2733
Conversation
Closing as this was already implemented in #2573. You can find more open issues that are good for new contributors at https://getporter.org/find-issue |
Sorry, as I saw this flag is absent into publish code! And it crush our pipelines which build and publish cnab. Because we can not able send build-arg when autobuild appeared. But let it be) |
Oops, thanks for pushing back, I thought I had updated the publish command in that pull request and see now that it was missed. I'll reopen and review. |
* The flag doesn't need to be manually configured with viper because the flag is defined on publish. * The flag doesn't need to be defined on Config.DataStore because the flag is defined on publish. * Update example to use --autobuild-disabled without =true, since that is the default when the flag is specified. Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
Add a test to validate that we can disable autobuild for the publish command. Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
@alekseybb197 Thank you for the fix! I have pushed 2 commits to your pull request to simplify how --autobuild-disabled is defined on publish and added a test to validate that the flag is working. Before I can merge, can you push a commit to your branch that includes the following text
This indicates you agree to our repository's DCO requirement. https://getporter.org/contribute/guide/#signing-your-commits |
/azp run porter-integration |
Azure Pipelines successfully started running 1 pipeline(s). |
Thanks @carolynvs , I try ... |
@carolynvs I see you remove "autobuild-disabled" from config file. Is it right? I'm only asking about the accepted decision. In other words, I do not need to return it with my commit? |
Correct, I removed AutoBuildDisabled from datastore.go because it is not necessary. The additional viper configuration in the cmd/porter package and declaring a config setting in DataStore is only needed for configuration values that are not defined as flags on a command. So please do not add it back. You can validate this locally by running |
Carolyn, with all due respect, I can't do this.ansible@test-vm:
ansible@test-vm: ansible@test-vm: ansible@test-vm: ansible@test-vm: |
Let me double check locally. I can also update the test to explicitly use config instead of the flag. |
I found a regression in binding config file values to flags in some circumstances. #2735 Once that is fixed (in-progress), then the autobuild-disabled config file setting should work as expected without the additional configuration (defining it on DataStore). |
Great! But on the other hand, I don’t know how to fill a commit with a signature :) |
One of your commits does have a signature, so I have marked the DCO as passing. When I merge, I will squash the commits into a single signed commit, co-authored by both of us. So everything is fine. I should have that config file regression fixed soon. Once that's merged, I'll update my publish test to use a config file (instead of a flag), and we can be sure that the original issue that you raised is addressed before I merge this PR. |
Thanks a lot Carolyn! Have a nice day! |
This adds a regression test for setting autobuild-disabled to true in a config file and having the CLI correctly pick up the setting. Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
/azp run porter-integration |
Azure Pipelines successfully started running 1 pipeline(s). |
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.
Thank you for your patience on this one! I appreciate the bug report and fix 👍
@@ -80,3 +80,4 @@ and we will add you. **All** contributors belong here. 💯 | |||
* [James Blair](https://github.com/jmhbnz) | |||
* [Chengwei Guo](https://github.com/cw-Guo) | |||
* [Sarah Christoff](https://github.com/hypernovasunnix) | |||
* [Aleksey Barabanov](https://github.com/alekseybb197) |
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.
🎉
* add addBundleDefinitionFlags to publish cmd * add AutoBuildDisabled to publish cmd * fix docs Signed-off-by: Aleksey Barabanov <Alexey.Barabanov@kaspersky.com> * Correct how --autobuild-disabled is defined on publish * The flag doesn't need to be manually configured with viper because the flag is defined on publish. * The flag doesn't need to be defined on Config.DataStore because the flag is defined on publish. * Update example to use --autobuild-disabled without =true, since that is the default when the flag is specified. Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com> * Add test for publish --autobuild-disabled Add a test to validate that we can disable autobuild for the publish command. Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com> * Add regression test for autobuild-disabled via config This adds a regression test for setting autobuild-disabled to true in a config file and having the CLI correctly pick up the setting. Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com> --------- Signed-off-by: Aleksey Barabanov <Alexey.Barabanov@kaspersky.com> Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com> Co-authored-by: Aleksey Barabanov <Alexey.Barabanov@kaspersky.com> Co-authored-by: Carolyn Van Slyck <me@carolynvanslyck.com>
What does this change
Added cli flag and configuration key for prevent unexpected autobuild invocation image during publish command because it crush pipelines execute.
Use as "porter publish ... --autobuild-disable" in cli or insert "autobuild-disable: true" into config.yaml.
What issue does it fix
Follow-up to #2573 which missed adding the flag to publish
Notes for the reviewer
Checklist
Reviewer Checklist