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: add delete-branch action #37

Merged
merged 7 commits into from
Jan 18, 2024
Merged

feat: add delete-branch action #37

merged 7 commits into from
Jan 18, 2024

Conversation

bethesque
Copy link
Contributor

@bethesque bethesque commented Nov 11, 2023

I have a few questions about how this works @YOU54F

  • The actions don't accept a password, but most of these work for the Pact Broker as well as Pactflow. Is there any reason to not support a password?
  • Why is the validation for the command inputs duplicated in the .sh files?
  • Is there a constraint to hardcode the tag to :latest? This seems a bit dangerous, and counter to the best practice we suggest for all dependencies, which is to pin them. Could we allow the tag as an input to the step?

@YOU54F
Copy link
Member

YOU54F commented Nov 13, 2023

The actions don't accept a password, but most of these work for the Pact Broker as well as Pactflow. Is there any reason to not support a password?

Correcto! The initial action was created by a contributor, and was for the Provider contract publishing which is PactFlow only. It has since been extended to include additional commands supported by both the OSS and PactFlow brokers.

There is no reason now, at least for other commands, to not support Basic and Token based authorization, so I would be happy for these to be updated.

However to avoid maintenance effort in needing to create multiple actions, I prefer to use the full cli directly as an single action, which can support anything exposed by the CLI - https://github.com/pactflow/actions/blob/main/action.yml

This standard action uses a cli.sh file, which can accept a PACT_CLI_VERSION

# Usage: (install fixed version) - pass PACT_CLI_VERSION=v<PACT_CLI_VERSION> eg PACT_CLI_VERSION=v1.92.0 or set as an env var

Why is the validation for the command inputs duplicated in the .sh files?

Legacy reasons - 7f3a1a4

Are you suggesting we just use the validation directly the CLI tool itself? Maybe the error messages wouldn't be as useful for the user, as it may say consumer version doesn't exist, but its called VERSION as part of an action.

Is there a constraint to hardcode the tag to :latest? This seems a bit dangerous, and counter to the best practice we suggest for all dependencies, which is to pin them. Could we allow the tag as an input to the step?

Yes, tag as an input to the step is a good idea, however we probably just want to be clear for a particular action, as which tag it exists from (for example the delete-branch will only exist in versions greater than https://hub.docker.com/layers/pactfoundation/pact-cli/0.56.0.4/images/sha256-55b6b7c210d0a2a34fa38f2de69f4b7f013c6d09e5e2cce2e76a22e000cf2c9b?context=explore)

note: we still need to release pact_broker-client with the delete-branch command in 👍🏾

@YOU54F YOU54F self-requested a review January 15, 2024 18:59
Copy link
Member

@YOU54F YOU54F left a comment

Choose a reason for hiding this comment

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

@YOU54F YOU54F merged commit 92669af into main Jan 18, 2024
28 checks passed
@YOU54F YOU54F deleted the feat/delete-branch branch September 20, 2024 12:03
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.

2 participants