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

Do not retry when --dry-run (or PACT_BROKER_CAN_I_DEPLOY_DRY_RUN) is provided #149

Closed
stan-is-hate opened this issue Oct 30, 2023 · 5 comments · Fixed by #169
Closed

Do not retry when --dry-run (or PACT_BROKER_CAN_I_DEPLOY_DRY_RUN) is provided #149

stan-is-hate opened this issue Oct 30, 2023 · 5 comments · Fixed by #169
Labels
good first issue Indicates a good issue for first-time contributors

Comments

@stan-is-hate
Copy link

stan-is-hate commented Oct 30, 2023

If you run can-i-deploy in dry-run mode but provide --retry-while-unknown value, the command would retry on the unknown verification results, same as without --dry-run.

I'd argue that if you're running in dry-run, you're not interested in the results anyway, so there's no point waiting for the verification results to become published.
Moreover, if you're running in dry run, there's a high chance that the verification result will never become available, since dry-run use cases include "break glass" situations (where you don't care and just want to deploy) or onboarding your systems to pact, where some services might delay the onboarding for whatever reason, so they won't upload any verifications.

So my suggestion would be to ignore --retry-while-unknown and --retry-interval flags if --dry-run is provided.

In fact, that's how --ignore/PACT_BROKER_CAN_I_DEPLOY_IGNORE already is, if you run can-i-deploy with both --ignore and --retry.. flags provided, it won't retry.

Alternatively, if you folks want to keep dry run as a true dry run, how about providing a way to ignore all pacts?
Either --ignore-all/PACT_BROKER_CAN_I_DEPLOY_IGNORE_ALL or --ignore="*" or something?

Thanks! I'd be happy to contribute, but have zero ruby knowledge.

@stan-is-hate
Copy link
Author

friendly ping on this one :)

@bethesque
Copy link
Member

Thanks for the suggestion. I can see the logic of your proposal. For now, can you please handle this in the script that calls the can-i-deploy, as I will not get to this issue any time soon unfortunately.

@YOU54F YOU54F added the good first issue Indicates a good issue for first-time contributors label Jun 18, 2024
@YOU54F
Copy link
Member

YOU54F commented Aug 12, 2024

Released in 1.76.0

Will be propagating through to the standalone and docker image shortly

@stan-is-hate
Copy link
Author

Thanks for fixing this! Is it possible to do the same for can-i-merge please?

@YOU54F
Copy link
Member

YOU54F commented Sep 10, 2024

It should work for can-i-merge as it calls CanIDeploy

def can_i_merge(*ignored_but_necessary)
require "pact_broker/client/cli/version_selector_options_parser"
require "pact_broker/client/can_i_deploy"
validate_credentials
selectors = VersionSelectorOptionsParser.call(ARGV)
validate_can_i_deploy_selectors(selectors)
dry_run = options.dry_run || ENV["PACT_BROKER_CAN_I_MERGE_DRY_RUN"] == "true"
can_i_merge_options = { output: options.output, retry_while_unknown: options.retry_while_unknown, retry_interval: options.retry_interval, dry_run: dry_run, verbose: options.verbose }
result = CanIDeploy.call(selectors, { with_main_branches: true }, can_i_merge_options, pact_broker_client_options)
$stdout.puts result.message
$stdout.flush
exit(1) unless result.success
end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Indicates a good issue for first-time contributors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants