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

[Enhancement] Manual start gradle check is required to merge anything #4

Closed
dblock opened this issue Apr 21, 2021 · 19 comments
Closed

Comments

@dblock
Copy link
Member

dblock commented Apr 21, 2021

Describe the bug

Before PR's are allowed to be merged the OpenSearch repository enforces running a CI task, ./gradlew check, which runs full unit, integration, smoke, and varying cluster tests. This takes a very long time and consumes a lot of resources. To avoid becoming a bottleneck we introduced a manual start gradle check that a maintainer can kick off by adding a comment to a PR once a basic review of a PR has been made. This is a manual step.

Expected behavior

No manual interaction from a maintainer other than approving/merging a PR.

Screenshots

Screen Shot 2021-04-21 at 3 19 12 PM

Additional context

@peterzhuamazon
Copy link
Member

Hi @dblock,

Thanks for the feedback on this.

I would not call it a bug, it is a design decision that we take, and for the time being it serves the CI by allowing admins to manually give a trigger to the check.

Automated trigger would add unnecessary blocks and checks such as running gradle check for pure documentation changes and such. It will also have issues when your previous check failed and you need to re-trigger it again.

We have discussed this approach many times, and for the time being it will keep this way until further discussion and better process being introduced.

Thanks.

@peterzhuamazon peterzhuamazon changed the title [BUG] Manual start gradle check is required to merge anything [Enhancement] Manual start gradle check is required to merge anything Apr 21, 2021
@dblock
Copy link
Member Author

dblock commented Apr 21, 2021

@peterzhuamazon a possible improvement related to this would be to allow admins to do skip gradle check for PRs that don't have code changes such as .md files. Right now I am keeping opensearch-project/OpenSearch#595 in draft collecting enough meaty changes to avoid wasting time in these

@peterzhuamazon
Copy link
Member

@dblock The CI never triggers until you start gradle check. The reason you see the check in check tab is due to GitHub allows you to label a check as required or not required. In order to prevent reviewers forgetting about the gradle check, we label it as required, but you can always overwrite the merge without running the check if you are repo admin.

As of now I dont see a way to label checks per PR, only per Repo.
Will need to research on it.

Thanks.

@dblock
Copy link
Member Author

dblock commented Apr 22, 2021

Thanks @peterzhuamazon for the details. I don't feel very strongly about any of this, just trying to reduce barriers for potential contributors.

@CEHENKLE CEHENKLE transferred this issue from opensearch-project/OpenSearch Sep 7, 2021
@CEHENKLE
Copy link
Member

CEHENKLE commented Sep 7, 2021

Moved to opensearch-build. Confirming, yes this is still a pain :(

@peternied
Copy link
Member

We've been cautious about the level of automation that is enabled by default - we will enable this and see if everything works well.

@peternied peternied assigned peternied and unassigned peternied Sep 14, 2021
@peternied peternied transferred this issue from opensearch-project/opensearch-build Sep 14, 2021
@peternied
Copy link
Member

@peterzhuamazon I'm OK pulling the trigger on this if you are, any concerns?

@CEHENKLE
Copy link
Member

I'm not Peter, but no time like the present to work things out :)

@peternied
Copy link
Member

The Peter hivemind has more openings, just requires a small name change

@dblock
Copy link
Member Author

dblock commented Oct 19, 2021

Please! I must have typed start gradle check a dozen times today.

@peterzhuamazon
Copy link
Member

@peterzhuamazon I'm OK pulling the trigger on this if you are, any concerns?

I do have concerns since this will autotrigger on EVERY COMMIT, not EVERY PR.

@dblock
Copy link
Member Author

dblock commented Oct 21, 2021

@peterzhuamazon I'm OK pulling the trigger on this if you are, any concerns?

I do have concerns since this will autotrigger on EVERY COMMIT, not EVERY PR.

There should be one gradle check running per PR as long as there are newer commits.

@peterzhuamazon
Copy link
Member

@peterzhuamazon I'm OK pulling the trigger on this if you are, any concerns?

I do have concerns since this will autotrigger on EVERY COMMIT, not EVERY PR.

There should be one gradle check running per PR as long as there are newer commits.

That would be how autotrigger work.

@peternied
Copy link
Member

peternied commented Oct 21, 2021

@peterzhuamazon I'm OK pulling the trigger on this if you are, any concerns?

I do have concerns since this will autotrigger on EVERY COMMIT, not EVERY PR.

@peterzhuamazon

Is this a problem with a lack of controls in the plugin, it seems like Build every pull request automatically without asking (Dangerous!). isn't every commit, but every PR am I misunderstanding this configuration? Do we need to test this to confirm?

Is there scalable way we can add external contributors to a opensearch-project org team / list that occurs to you. E.g. is there a command like allow pull requests by dblock to run automatically that could add dblock to a list. I think there is do we want to try that first?

@peterzhuamazon
Copy link
Member

@peterzhuamazon I'm OK pulling the trigger on this if you are, any concerns?

I do have concerns since this will autotrigger on EVERY COMMIT, not EVERY PR.

@peterzhuamazon

Is this a problem with a lack of controls in the plugin, it seems like Build every pull request automatically without asking (Dangerous!). isn't every commit, but every PR am I misunderstanding this configuration? Do we need to test this to confirm?

Is there scalable way we can add external contributors to a opensearch-project org team / list that occurs to you. E.g. is there a command like allow pull requests by dblock to run automatically that could add dblock to a list. I think there is do we want to try that first?

It will build once the second you create a PR, then build on every new commit coming in.

@dblock
Copy link
Member Author

dblock commented Oct 21, 2021

I forget, do those gradle workers have access to credentials? Just double-checking, if they do, we can't auto-enable it (GHA does not give access to secrets on pull requests).

@peterzhuamazon
Copy link
Member

I forget, do those gradle workers have access to credentials? Just double-checking, if they do, we can't auto-enable it (GHA does not give access to secrets on pull requests).

I do not think it is having access to credentials.
The only thing it does is pull the repo then run gradle check.

@peternied
Copy link
Member

I am tempted to uncheck this setting, @peterzhuamazon will this start triggering builds for allow listed users but still require the start gradle check for everyone else? It seems like the right thing, but I know you spend time looking into the options.

image

@peterzhuamazon
Copy link
Member

After agreeing with both @dblock and @peternied we have removed the trigger and have Gradle Check autorun for EVERY SINGLE COMMIT in PRs.

Thanks.

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

No branches or pull requests

4 participants