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

feat: support --ignore flag in can-i-merge command #174

Closed

Conversation

stan-is-hate
Copy link
Contributor

This should bring it up-to-date with can-i-deploy.
Tested by unit test and also by building it and running locally against dockerized broker.

@@ -66,7 +66,7 @@ Options:
# The consumer application version
-h, [--branch=BRANCH]
# Repository branch of the consumer version
-r, [--auto-detect-version-properties], [--no-auto-detect-version-properties]
-r, [--auto-detect-version-properties], [--no-auto-detect-version-properties], [--skip-auto-detect-version-properties]
Copy link
Member

Choose a reason for hiding this comment

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

What's going on with all of these --skip-.. variants?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just ran a command from README to re-generate docs, so I guess someone added these flags but never re-generated the docs

Copy link
Member

Choose a reason for hiding this comment

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

Thanks Stan. @YOU54F @pahnin any ideas as to why these --skip things are appearing? It's not entirely clear how they're different to the --no-... options.

Copy link
Member

@YOU54F YOU54F Oct 23, 2024

Choose a reason for hiding this comment

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

Looks like it came from this commit, (documenting of the option), which was released in thor 1.3.1. Downgrading thor to 1.3.0 and running ruby script/update-cli-usage-in-readme.rb will generate the readme without the --skip* opts

rails/thor@686bcd5

The option itself has been there since 2009 - commit

effectively --no-foo is the same as --skip-foo

Copy link
Member

Choose a reason for hiding this comment

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

You can rebase against master now as I've performed that readme update in a separate PR

#176

lib/pact_broker/client/cli/matrix_commands.rb Show resolved Hide resolved
@@ -121,6 +123,9 @@ def validate_can_i_deploy_options
def ignore_selectors_from_environment_variable
ENV.fetch("PACT_BROKER_CAN_I_DEPLOY_IGNORE", "").split(",").collect(&:strip).collect{ |i| { pacticipant: i } }
end
def ignore_merge_selectors_from_environment_variable
ENV.fetch("PACT_BROKER_CAN_I_MERGE_IGNORE", "").split(",").collect(&:strip).collect{ |i| { pacticipant: i } }
Copy link
Member

Choose a reason for hiding this comment

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

Right, yes can't specify versions here :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just copied the behavior of can-i-deploy ignore flag/var :)

spec/integration/can_i_merge_spec.rb Show resolved Hide resolved
@YOU54F
Copy link
Member

YOU54F commented Oct 23, 2024

Looks good to me, I've updated the proposed test and added a new one to test the ignore logic, whereby a broker result contains 2 indiv verification results for 2 consumers, 1 failing, 1 passing.

see #177 (including a rebase against master to avoid the additional readme update noise)

In one case, the broker returns an ignored verification, the matrix lists it, and exits successfully.

In the other case, the broker returns an undeployable verification, and computer says now

@stan-is-hate
Copy link
Contributor Author

Thanks @YOU54F ! With your PR in flight, should we close this one?

@YOU54F
Copy link
Member

YOU54F commented Oct 24, 2024

merged as part of #177, ty

@YOU54F YOU54F closed this Oct 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants