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

Jump to bottom

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

Merged
merged 3 commits into from Sep 24, 2021
Merged

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

merged 3 commits into from Sep 24, 2021

Conversation

rabraghib
Copy link
Contributor

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 a review from a team September 12, 2021 01:13
@rabraghib rabraghib requested a review from a team as a code owner September 12, 2021 01:13
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. 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
@rabraghib rabraghib mentioned this pull request Sep 12, 2021
Closed
@bcoe
Copy link
Contributor

bcoe commented Sep 13, 2021

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

@rabraghib
Copy link
Contributor Author

thanks @bcoe for your feedback!!

@rabraghib
Copy link
Contributor Author

@joeldodge79

joeldodge79
joeldodge79 requested changes Sep 23, 2021
View reviewed changes
Copy link
Collaborator

@joeldodge79 joeldodge79 left a comment

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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 •
edited

Choose a reason for hiding this comment

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

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

joeldodge79
joeldodge79 approved these changes Sep 24, 2021
View reviewed changes
Copy link
Collaborator

@joeldodge79 joeldodge79 left a comment

Choose a reason for hiding this comment

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

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

@rabraghib
Copy link
Contributor Author

@joeldodge79 fixed.

@bcoe bcoe merged commit 59f3094 into googleapis:main Sep 24, 2021
@bcoe
Copy link
Contributor

bcoe commented Sep 24, 2021

@rabraghib thank you for the contribution.

gcf-merge-on-green bot pushed a commit that referenced this pull request Sep 24, 2021
@release-please
chore: release 12.3.0 (#1071)
3e0c263 
馃 I have created a release \*beep\* \*boop\*
---
## [12.3.0](https://www.github.com/googleapis/release-please/compare/v12.2.0...v12.3.0) (2021-09-24)


### Features

* allow forcing latest tag ([#1070](https://www.github.com/googleapis/release-please/issues/1070)) ([0549a30](https://www.github.com/googleapis/release-please/commit/0549a3035c8348c62958d2f1f037226bf2a0ce21))
* **manifest:** add option to skip creating github release ([#1048](https://www.github.com/googleapis/release-please/issues/1048)) ([59f3094](https://www.github.com/googleapis/release-please/commit/59f309429586200f835fdffe07ed9860a1901e31))
* **python:** support src/packagename/__init__.py ([#1062](https://www.github.com/googleapis/release-please/issues/1062)) ([598667d](https://www.github.com/googleapis/release-please/commit/598667da5a623c3fb057874840b3c308d225c627))
---


This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bcoe bcoe bcoe approved these changes

@joeldodge79 joeldodge79 joeldodge79 approved these changes

Assignees

@joeldodge79 joeldodge79

Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow skipping release part for a specific package for manifest command
3 participants
@rabraghib @bcoe @joeldodge79