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

[skip-ci] GHA: Fix intermittent workflow error #22304

Merged
merged 1 commit into from
Apr 9, 2024

Conversation

cevich
Copy link
Member

@cevich cevich commented Apr 8, 2024

Periodically, the discussion-lock workflow throws the error: Resource not accessible by integration

This was identified in the upstream issue 47, as caused by a version-5 change that adds support for management of discussions but requires additional permissions and possibly settings. Given the low notification traffic from discussions, old discussions may remain valid for a long while, and are a useful community-interface: Disable management of discussions.

Does this PR introduce a user-facing change?

None

@openshift-ci openshift-ci bot added release-note-none do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Apr 8, 2024
@cevich cevich marked this pull request as ready for review April 8, 2024 15:18
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 8, 2024
@cevich
Copy link
Member Author

cevich commented Apr 8, 2024

@edsantiago @lsm5 @mheon PTAL, this should fix the alerts we've been getting from this repo. There's no practical way to test this PR, so it needs to be manually merged (the bot will not work). If acceptable, I intend to open similar PRs for Buildah and Skopeo where we also receive this alert.

@cevich
Copy link
Member Author

cevich commented Apr 8, 2024

Clarification: "The fix worked for other people" is all I have to go on for this change.

Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

Let's please disable the thing for discussions instead, they are discussions and not meant locked just because someone closed them IMO.
dessant/lock-threads#47 (comment)

And if we do want to lock them I guess we must configure it the same way we did for PRs/issues so that it does not use the defaults.

@cevich
Copy link
Member Author

cevich commented Apr 8, 2024

Thanks for your input Paul, sorry I probably should have tagged you as well.

IIRC, the original concern was over receiving e-mail notifications relating to old closed PRs/Issues. So these would not be open/active discussions.

The workflow is already configured to use the same settings as for issues/PRs (and now discussions).

@Luap99
Copy link
Member

Luap99 commented Apr 8, 2024

IIRC, the original concern was over receiving e-mail notifications relating to old closed PRs/Issues. So these would not be open/active discussions.

Yes exactly which is why this change is wrong because you enable locking of discussions, IMO discussions are from and for community and we see plenty of non maintainers active there as well, there is no requirement for us to interact with them.
Discussions are also sorted by on activity so if someone is active on a old discussion then it will be back at the font and be discoverable for others to join in.

The workflow is already configured to use the same settings as for issues/PRs (and now discussions).

issue-inactive-days: '${{env.CLOSED_DAYS}}'
pr-inactive-days: '${{env.CLOSED_DAYS}}'
add-issue-labels: '${{env.LOCKED_LABEL}}'
add-pr-labels: '${{env.LOCKED_LABEL}}'

How do you set these for discussion? You currently don't so this fix would make things worse as it uses the defaults (which we do not liked)

@cevich cevich force-pushed the fix_res_inacs_by_int branch from 99b0e8c to 145e9c9 Compare April 8, 2024 19:19
@cevich
Copy link
Member Author

cevich commented Apr 8, 2024

Force-pushed: Added config. for discussions.

Note: I'm open to disabling management of discussions, assuming that's what the team wants. I'll make a note to bring this up.

@cevich cevich force-pushed the fix_res_inacs_by_int branch from 145e9c9 to e2d8074 Compare April 9, 2024 14:44
@cevich
Copy link
Member Author

cevich commented Apr 9, 2024

Force-push: Disabled discussion-management, renamed job to better reflect this.

@Luap99 @mheon PTAL, this should be ready to go now.

Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

LGTM although I think if you change the name there you want to change it in the other place as well.

.github/workflows/discussion_lock.yml Outdated Show resolved Hide resolved
Copy link
Contributor

openshift-ci bot commented Apr 9, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cevich, Luap99

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 9, 2024
Periodically, the discussion-lock workflow throws the error: `Resource
not accessible by integration`

This was identified in the
[upstream](https://github.com/dessant/lock-threads)
issue 47, as caused by a version-5 change that adds support for
management of discussions but requires additional permissions
and possibly settings.  Given the low notification traffic from
discussions, old discussions may remain valid for a long while, and are
a useful community-interface:  Disable management of discussions.

Signed-off-by: Chris Evich <[email protected]>
@cevich cevich force-pushed the fix_res_inacs_by_int branch from e2d8074 to 7f0268a Compare April 9, 2024 15:19
@cevich
Copy link
Member Author

cevich commented Apr 9, 2024

Force-push: Renamed job and workflow file.

Copy link

Cockpit tests failed for commit 7f0268a. @martinpitt, @jelly, @mvollmer please check.

@edsantiago
Copy link
Member

LGTM. Looking forward to Thursday or Friday to see if this fixes the problem. Thank you!

@edsantiago
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 9, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 918e4a3 into containers:main Apr 9, 2024
93 of 94 checks passed
cevich added a commit to cevich/buildah that referenced this pull request Apr 9, 2024
Apply the same change as in
containers/podman#22304

Signed-off-by: Chris Evich <[email protected]>
@cevich
Copy link
Member Author

cevich commented Apr 9, 2024

Looking forward to Thursday or Friday to see if this fixes the problem. Thank you!

Note: Both buildah and skopeo re-use this workflow via GHA magic. So by merging this we should find the problem is fixed everywhere. Hopefully.

@cevich
Copy link
Member Author

cevich commented Apr 9, 2024

Oops, nope, spoke too soon. The file rename is going to cause both Skopeo and Buildah to fail.

cevich added a commit to cevich/buildah that referenced this pull request Apr 9, 2024
cevich added a commit to cevich/buildah that referenced this pull request Apr 9, 2024
cevich added a commit to cevich/buildah that referenced this pull request Apr 9, 2024
cevich added a commit to cevich/skopeo that referenced this pull request Apr 9, 2024
@edsantiago
Copy link
Member

Good idea for a followup: add a huge all-caps warning saying "THIS FILE IS USED BY SKOPEO AND BUILDAH etc etc"

@cevich
Copy link
Member Author

cevich commented Apr 9, 2024

That is a good idea.

@stale-locking-app stale-locking-app bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Jul 9, 2024
@stale-locking-app stale-locking-app bot locked as resolved and limited conversation to collaborators Jul 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants