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

8263204: Add Gradle Wrapper Validation Action #419

Closed
wants to merge 4 commits into from

Conversation

jgneff
Copy link
Member

@jgneff jgneff commented Mar 8, 2021

See the Gradle Wrapper Validation Action for details on this pull request. I'll test the changes with the following sequence of commits:

  1. This commit adds a tampered Gradle Wrapper JAR file, which should go undetected.
  2. The next commit will add the Official Gradle Wrapper Validation Action, which should detect the tampered file.
  3. The final commit will remove the tampered file and replace it with the original Gradle 4.8 Wrapper.

Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jfx pull/419/head:pull/419
$ git checkout pull/419

@bridgekeeper
Copy link

bridgekeeper bot commented Mar 8, 2021

👋 Welcome back jgneff! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr Ready for review label Mar 8, 2021
@kevinrushforth
Copy link
Member

  1. This commit adds a tampered Gradle Wrapper JAR file, which should go undetected.
  2. The next commit will add the Official Gradle Wrapper Validation Action, which should detect the tampered file.
  3. The final commit will remove the tampered file and replace it with the original Gradle 4.8 Wrapper.

This sounds like a good plan to test it.

@mlbridge
Copy link

mlbridge bot commented Mar 8, 2021

Webrevs

@kevinrushforth kevinrushforth self-requested a review March 8, 2021 20:38
@jgneff
Copy link
Member Author

jgneff commented Mar 8, 2021

So far, so good. The tampered file was not detected:

all-checks-have-passed

The next commit will add the Official Gradle Wrapper Validation Action.

@kevinrushforth
Copy link
Member

It might be better to include the validation task in the same submit.yml file as the pre-submit tests, as a separate job. That way it will get the same set of conditions triggering it as the other pre-submit jobs. In particular, we don't use the "on pull_request" trigger for our github actions run, since all actions triggered on any pull request in any repo in the openjdk org will be run in the context of the openjdk organization and we would blow our limits too quickly. Also, this should be limited to the set of branches that submit.yml uses.

If there is a good reason to keep it in a separate file, then I would at least duplicate this part from submit.yml:

on:
  # Run GitHub actions on every push to all branches except the main production branches, also
  # exclude any branch starting with "WIP".
  push:
    branches-ignore:
      - master
      - main
      - 'jfx[0-9]+'
      - 'WIP*'

@jgneff
Copy link
Member Author

jgneff commented Mar 8, 2021

Thanks, Kevin. I'll merge the two workflow files.

The test results aren't what I expected:

some-checks-were-not-successful

I assumed the wrapper validation would fail and prevent the builds from running. Should I try to make the three build steps dependent on the success of the wrapper validation? There might be a way to make the steps sequential and conditional.

@kevinrushforth
Copy link
Member

If it isn't too hard to make the running of the other three jobs dependent on the wrapper validation job, that would be good, as long as the validation job is fast. The other three still need to run in parallel with each other.

Alternatively, you could make the wrapper validation a task that is replicated in each of the three jobs.

Consolidate the two GitHub workflow files and make the three build jobs
dependent on the success of the Gradle Wrapper validation.
@jgneff
Copy link
Member Author

jgneff commented Mar 8, 2021

Okay, this is looking better! The validation took only 23 seconds, and GitHub shows the following results:

some-checks-were-not-successful-2

The screenshot below shows the notification I received by e-mail:

run-failed-message

Now we'll see whether the three build jobs still run in parallel when the wrapper validation succeeds. My next commit will restore the Gradle Wrapper JAR file.

@jgneff
Copy link
Member Author

jgneff commented Mar 8, 2021

Looks good: two preliminary checks followed conditionally by the three concurrent build jobs:

some-checks-haven’t-completed-yet

Once the build jobs finish, this pull request is finally ready for review. Thanks for your timely help, Kevin.

@kevinrushforth
Copy link
Member

Yep, they are running in parallel. And this provides a nice view:

https://github.com/jgneff/jfx/actions/runs/633879096

@openjdk
Copy link

openjdk bot commented Mar 9, 2021

@jgneff This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8263204: Add Gradle Wrapper Validation Action

Reviewed-by: kcr

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@kevinrushforth) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready Ready to be integrated label Mar 9, 2021
@jgneff
Copy link
Member Author

jgneff commented Mar 9, 2021

/integrate

@openjdk openjdk bot added the sponsor Ready to sponsor label Mar 9, 2021
@openjdk
Copy link

openjdk bot commented Mar 9, 2021

@jgneff
Your change (at version 9a4b021) is now ready to be sponsored by a Committer.

@kevinrushforth
Copy link
Member

/sponsor

@openjdk openjdk bot closed this Mar 9, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed sponsor Ready to sponsor ready Ready to be integrated rfr Ready for review labels Mar 9, 2021
@openjdk
Copy link

openjdk bot commented Mar 9, 2021

@kevinrushforth @jgneff Pushed as commit 75b4c15.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@jgneff jgneff deleted the gradle-wrapper-validation branch March 21, 2021 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated
2 participants