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 afterRun hook #2182

Merged
merged 1 commit into from
Feb 10, 2023
Merged

Conversation

jBouyoud
Copy link
Contributor

@jBouyoud jBouyoud commented Mar 24, 2022

What Changed

Add new afterRun hook triggered after each command.

Why

Because beforeRun exists ;-P

Todo:

  • Add tests
  • Add docs

Change Type

Indicate the type of change your pull request is:

  • documentation
  • patch
  • minor
  • major

@jBouyoud jBouyoud marked this pull request as ready for review March 24, 2022 07:38
@jBouyoud
Copy link
Contributor Author

Just like #2180 , I didn't understand why ci check pr-check is failling with a 404.

@hipstersmoothie
Copy link
Collaborator

I think this would mean that we have to update anywhere we use the afterShipIt hook since right now we assume we are shipping something. This would also be a breaking change too since it could break people using the hook

@jBouyoud
Copy link
Contributor Author

jBouyoud commented Apr 5, 2022

I think this would mean that we have to update anywhere we use the afterShipIt hook since right now we assume we are shipping something. This would also be a breaking change too since it could break people using the hook

This is not really a breaking change since the hook payload permit it and the purpose of this hook seems to be : Ran after the 'shipit' command has run. So all plugins must handle the payload properly.
But of course if the intent of this hook was to be only called when a release is done 🤷 .

I see two options :

  1. Check all existing afterShipIt hooks to properly handle newVersion === undefined
  2. Create a new hooks afterRun, that can be called after all commands (just like beforeRun). And change newVersion to only a string and update afterShipIt description to intent this.

I'll be glad to handle this. Please let me known what is your preference ?

@intuit intuit deleted a comment from Procorners May 20, 2022
@intuit intuit deleted a comment from Procorners May 20, 2022
@hipstersmoothie
Copy link
Collaborator

@jBouyoud Route 2 sounds like a great option! I like how we would then have the beforeRun/afterRun combo.

@jBouyoud jBouyoud marked this pull request as draft May 30, 2022 15:56
@jBouyoud jBouyoud changed the title Fix auto/core always run afterShipIt hook even when no release is done Feat add afterRun hook May 30, 2022
@intuit-svc intuit-svc added the minor Increment the minor version when merged label May 30, 2022
@jBouyoud jBouyoud force-pushed the always-run-after-shipit branch 3 times, most recently from dfa44e1 to ed64c9d Compare May 31, 2022 09:18
@jBouyoud jBouyoud marked this pull request as ready for review May 31, 2022 09:30
@jBouyoud
Copy link
Contributor Author

@hipstersmoothie please find an implementation proposal for the route 2

@jBouyoud
Copy link
Contributor Author

@hipstersmoothie anything that I can do to help merge this PR ?

@jBouyoud
Copy link
Contributor Author

jBouyoud commented Feb 8, 2023

@hipstersmoothie I also rebase this PR ;-P

@jBouyoud
Copy link
Contributor Author

jBouyoud commented Feb 9, 2023

pr-check, release seems fails because : PR env variable is empty ; probably related to this.

And github actions permissions are all in read even the workflow file specify write permissions 🤷

@jBouyoud
Copy link
Contributor Author

@hipstersmoothie Let me know if everything miss to merge this

@hipstersmoothie hipstersmoothie merged commit 5d9dd37 into intuit:main Feb 10, 2023
@github-actions
Copy link

🚀 PR was released in v10.42.0 🚀

@github-actions github-actions bot added the released This issue/pull request has been released. label Feb 10, 2023
@jBouyoud jBouyoud deleted the always-run-after-shipit branch February 13, 2023 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor Increment the minor version when merged released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants