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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(manifest): add option to skip creating github release #1048

Merged
merged 3 commits into from Sep 24, 2021

Conversation

@rabraghib
Copy link
Contributor

@rabraghib rabraghib commented Sep 12, 2021

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #1042 馃

Here is a simple repo that shows the functionality: https://github.com/rabraghib/release-please-tests

@rabraghib rabraghib requested review from as code owners Sep 12, 2021
@google-cla google-cla bot added the cla: yes label Sep 12, 2021
@rabraghib rabraghib changed the title feat(manifest): add oprion to skip creating github release feat(manifest): add option to skip creating github release Sep 12, 2021
@bcoe
Copy link
Contributor

@bcoe bcoe commented Sep 13, 2021

@rabraghib this looks quite reasonable to me, will let @joeldodge79 chime in too.

@rabraghib
Copy link
Contributor Author

@rabraghib rabraghib commented Sep 13, 2021

thanks @bcoe for your feedback!!

@rabraghib
Copy link
Contributor Author

@rabraghib rabraghib commented Sep 23, 2021

Copy link
Collaborator

@joeldodge79 joeldodge79 left a comment

sorry for the lag (missed the notification). Thanks for the contribution, I like the feature! I have a suggestion (inline) for slightly different behavior, let me know what you think.

Also, could you add a test or two?

src/manifest.ts Outdated
[{sha: lastMergedPR.sha, message: '', files: lastMergedPR.files}],
lastMergedPR.sha
)
).filter(pkg => !pkg.config.skipGithubRelease);
Copy link
Collaborator

@joeldodge79 joeldodge79 Sep 23, 2021

What do you think about a slightly different approach: rather than just exclude them what if we checked this flag further down and did this

this.gh.commentOnIssue("`:robot: ${pkgName} not configured for release :no_entry_sign:`")

Easier for after-the-fact troubleshooting (rather than looking at git history on release-please-config.json and tying the timeline to release creation runs)

I also think it handles this edge case better: all packages are marked as skip-github-release - that way we still "release" the PR (remove the autorelease: pending label and add the autorelease: tagged label). That doesn't really affect functionality (we only ever look at the lastMergedPR) but for historical viewing/reporting I think it makes more sense to see a "released" PR and it just so happens that no package was actually released.

Copy link
Contributor Author

@rabraghib rabraghib Sep 23, 2021

Yp I totally agree.
I will update it & I will try adding some tests

Copy link
Collaborator

@joeldodge79 joeldodge79 left a comment

Looks good! seems like there's a file conflict that needs fixing though

@rabraghib
Copy link
Contributor Author

@rabraghib rabraghib commented Sep 24, 2021

@joeldodge79 fixed.

bcoe
bcoe approved these changes Sep 24, 2021
@bcoe bcoe merged commit 59f3094 into googleapis:main Sep 24, 2021
12 checks passed
@bcoe
Copy link
Contributor

@bcoe bcoe commented Sep 24, 2021

@rabraghib thank you for the contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants