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

FEDX-1399: Added validate publish to checks #13

Merged
merged 3 commits into from
Aug 8, 2024

Conversation

matthewnitschke-wk
Copy link
Contributor

@matthewnitschke-wk matthewnitschke-wk commented Aug 7, 2024

FEDX-1399

Issue Status

Currently its possible to merge a release PR in a state that will fail dart pub publish

This PR runs a publish validation for only release PRs so we don't get silent errors

@aviary-wf
Copy link

Security Insights

No security relevant content was detected by automated scans.

Action Items

  • Review PR for security impact; comment "security review required" if needed or unsure
  • Verify aviary.yaml coverage of security relevant code

Questions or Comments? Reach out on Slack: #support-infosec.

@matthewnitschke-wk matthewnitschke-wk marked this pull request as ready for review August 8, 2024 19:34
@bender-wk bender-wk changed the title Added validate publish to checks FEDX-1399: Added validate publish to checks Aug 8, 2024
@matthewnitschke-wk matthewnitschke-wk requested a review from a team August 8, 2024 19:39
validate-publish:
runs-on: ubuntu-latest
# only run on release pull requests
if: ${{ startsWith(github.ref, 'refs/head/release') && github.event.sender == 'rmconsole-readonly-wk' }}

Choose a reason for hiding this comment

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

Is it a guarantee that the sender will be rmconsole-readonly-wk? I thought we had multiple service accounts and that any of them could be involved based on rate limiting availability

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like we have 9 of them: https://github.com/orgs/Workiva/people?query=rmconsole

I guess I thought that readonly was the one that was used for release prs, although, that might be invalid

We could update that to contains(github.event.sender, 'rmconsole'), probably would be a better check

Choose a reason for hiding this comment

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

Maybe we can find a different/better way to identify release PRs? Technically, it's possible and totally valid from RM's perspective to manually create a release PR - as long as it meets a couple of criteria, RM will recognize it as such even if it didn't create the PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, from the perspective of gha-dart-oss, we want to validate for any PR that results in a tag

I'm not sure we'd be able to know that without hitting some, probably non-existant, rmconsole api

An alternate idea would be any PR that changes pubspec.yaml#version? would require a bit of introspection into the PR in question though

@matthewnitschke-wk
Copy link
Contributor Author

QA +1

  • CI does indeed pass

🚀 @Workiva/release-management-p 🚢

Copy link

@rmconsole-wf rmconsole-wf left a comment

Choose a reason for hiding this comment

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

+1 from RM

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

Successfully merging this pull request may close these issues.

5 participants