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

Add a workflow that safely runs after Format fails #50

Merged
merged 1 commit into from
Apr 25, 2023

Conversation

jmatsu
Copy link
Contributor

@jmatsu jmatsu commented Apr 24, 2023

Ref: #40

As we discussed, we should have safe code-format reminders. Please feel free to ask me anything for the clarification.

Examples: #52 (comment)


steps:
- id: linked-pull-request
uses: actions/github-script@060d68304cc19ea84d828af10e34b9c6ca7bdb31
Copy link
Member

@takahirom takahirom Apr 25, 2023

Choose a reason for hiding this comment

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

Defining the commit hash for GHA can enhance security, but it might also result in frequent updates from Renovate, and with auto-merge enabled, we might inadvertently incorporate unreleased changes, don't you think? It may be a tradeoff. 👀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't have to apply Actions' updates frequently so it's better to configure relaxed values in Renovate. Auto-merge is of course great as you said though, I think once per month is enough as for Actions with manual operations.

Copy link
Member

Choose a reason for hiding this comment

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

I'm sorry for my lack of knowledge. What measures do you take to avoid including unreleased changes when updating actions? How do you ensure that Renovate updates only apply to released commits? Are there any specific strategies or practices you follow? 👀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How do you ensure that Renovate updates only apply to released commits?

Sorry? I'm not sure this could be an answer to your question. Renovate will use SHAs of tags or branches' HEAD like v1.

What measures do you take to avoid including unreleased changes when updating actions?
Are there any specific strategies or practices you follow?

I have a simple regulation to include changes but not for the avoidance. As personal thoughts, I cannot find any notable value from including every update of Actions to our workflows automatically as long as they succeed.

  • We must include security fixes as soon as possible, however, we shouldn't lightly depend on scheduled bots for the purpose. We have to use some security features like dependabot alerts.
  • Refs are fragile. They are not unmodifiable so we may use untrusted changes and our workflows may suddenly fail without any modifications from our-side. Both behaviours are critical for us.
  • Basically, the merits of automated update are not high compared to libraries. We have to activate new valuable features like caching by tweaking configurations because the most of Actions's authors must avoid to change/break the existing workflows.

Copy link
Member

@takahirom takahirom Apr 25, 2023

Choose a reason for hiding this comment

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

Sorry? I'm not sure this could be an answer to your question. Renovate will use SHAs of tags or branches' HEAD like v1.

So, if it's the main branch, Renovate uses the main branch, right? This seems like it could be a snapshot build, right?
I took a look, and I found a way to use tag pinning. If we use it, it seems that we could use both hashes and tags.
https://github.com/renovatebot/renovate/pull/14110/files#diff-b0985a05bfddf6bf707b84953b7365a8aa45c1e510872f76f5f445f166806616R27

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

@takahirom takahirom left a comment

Choose a reason for hiding this comment

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

I left some comments. However, I believe it is better to include this new workflow in the repository! Thanks!


const headBranch = '${{ github.event.workflow_run.head_repository.fork && format('{0}:{1}', github.event.workflow_run.head_repository.owner.login, github.event.workflow_run.head_branch) || github.event.workflow_run.head_branch }}';

const { data: pulls } = await github.rest.pulls.list({
Copy link
Member

Choose a reason for hiding this comment

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

If someone opens a PR while executing the format, could this workflow comment on the wrong 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.

It's slim to none. Only two rare cases can cause the similar situation that you mentioned.

  1. Contributors and/or maintainers closed a previous pull request that transitively had triggered this workflow and would open a new pull request so quickly. i.e. base branches of pull requests were same.
  • In this case, this workflow will post comments to a new pull request. This is apparently correct, fortunately.
  1. Contributors and/or maintainers opened a new another pull request while a previous pull request was open. i.e. base branches of pull requests were different.
  • A new pull request will get comments in this case. This result is wrong if authors want comments to the older pull request.

Honestly speaking, we can ignore Case 2 above because there is no practical situation for it. Anyway, this workflow won't post any comments to unrelated pull requests.

@jmatsu
Copy link
Contributor Author

jmatsu commented Apr 25, 2023

Thank you for your review~

@jmatsu jmatsu merged commit 4a10c0b into main Apr 25, 2023
2 checks passed
@jmatsu jmatsu deleted the jmatsu/chore/notify_on_specific_workflow_failure branch April 25, 2023 04:50
@jmatsu jmatsu mentioned this pull request Apr 25, 2023
1 task
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.

None yet

2 participants