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 Dependency Review action in merge queue #180

Closed
wants to merge 1 commit into from

Conversation

rosstimothy
Copy link
Contributor

Various attempts to skip the action were tried in teleport.e, but they all resulted in the Dependency Review check never reporting a status. This blocked all PRs from landing until the merge queue was disabled. By defining the skip here instead of in an upstream repo it should prevent the aforementioned issue.

Various attempts to skip the action were tried in `teleport.e`, but
they all resulted in the Dependency Review check never reporting a
status. This blocked all PRs from landing until the merge queue was
disabled. By defining the skip here instead of in an upstream repo
it should prevent the aforementioned issue.
@rosstimothy rosstimothy requested review from a team November 13, 2023 13:50
@jentfoo
Copy link
Contributor

jentfoo commented Nov 13, 2023

@reedloden I was curious how this was working for teleport, and I see we never re-required Dependency Review after disabling in January due to a couple rust issues: https://github.com/gravitational/github-terraform/commit/fe2b03a99c6b

Unfortunately only one of the linked PRs has been merged. Should we require Dependency Review on e? I feel like whatever strategy we do for e should be in sync with teleport. Alternatively we could start to require it again on both and just disable the next time we have Rust work. Just brainstorming, let me know your thoughts.

@reedloden
Copy link
Contributor

Can we just add oid-registry to the ignored list for now?

As far as merge queue goes, I suspect we need to do something like this:
actions/dependency-review-action#456 (comment)

@rosstimothy
Copy link
Contributor Author

@reedloden @jentfoo is making the dependency check required in gravitational/teleport needed for this to land? Is that something we can address separately? Enabling the merge queue in gravitational/teleport.e would solve a few dev experience issues.

@jakule
Copy link

jakule commented Nov 17, 2023

@reedloden @jentfoo Friendly ping

@rosstimothy
Copy link
Contributor Author

@reedloden could you please weigh in? Is this ok to merge without addressing your concerns about the check still not being required in gravitational/teleport?

@reedloden
Copy link
Contributor

So, if we need to land this to unbreak something, we can, but this is not the correct fix. We need this action to run within the merge queue.

I'd rather us re-enable dependency review everywhere and just explicitly allowlist those rust packages for now.

@rosstimothy
Copy link
Contributor Author

@reedloden the problem is that the Dependency Review in its current form does not work in the merge queue, see https://gravitational.slack.com/archives/C0351AUPJ8K/p1699371871519019 for more details. This prevents us from enabling the merge queue in any repository that has the Dependency Review as a required check (currently only gravitational/teleport.e). If we were to make the Dependency Review as a required check everywhere, it would prevent gravitational/teleport (the only repository utilizing the merge queue) from landing any PRs.

The intent of this PR is to bypass the Dependency Review in the merge queue until @jentfoo is able to address the issues that the check has when triggered by the merge queue. A PR from a repository with the Dependency Review as a required check can only enter the merge queue if the check already passed. A PR from a repository that doesn't have the Dependency Review as a required check will not trigger the check when it enters the merge queue.

@reedloden
Copy link
Contributor

I just committed #183 and #184, which I think will help here. I've re-enabled merge queue in teleport.e.

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.

5 participants