Skip to content
This repository has been archived by the owner on Jul 19, 2023. It is now read-only.

Prevents the use of actions/setup-java@v1 and adopt /adopt-hotspot (openjdk) distributions. #82

Merged
merged 4 commits into from
Oct 4, 2022

Conversation

jmj0502
Copy link
Contributor

@jmj0502 jmj0502 commented Sep 29, 2022

Solves #26

  • Adds the prevent-action-setup-java-v1 rule. It prevents the use of the action setup-java@v1, since it doesn't allow uses to specify the distribution they'll use.
  • Adds the prevent-adopt-distributions-on-setup-java rule. It prevents the use of adopt/adopt-hotspot distributions since such distributions won't be supported anymore.

…on/setup-java@v1 since it doesn't support eclipse temurin distributions.
…opt-hotspot distributions on the actions/setup-java@v2+, since such distributions won't be supported anymore.
@jmj0502
Copy link
Contributor Author

jmj0502 commented Sep 29, 2022

@OriYosef Is there a way to test the new rules? The contribution section doesn't contain info on how to perform tests on rules.

@@ -0,0 +1,7 @@
{
"description": "Prevent use of action/setup-java@v1.",
"failureMessage": "action/setup-java@v1 is used on the workflow.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Please change the failureMessage here as well

@OriYosef
Copy link
Contributor

OriYosef commented Sep 30, 2022

Hey @jmj0502 . Thank you again for your contribution!
I have left some comments on your rules.
About testing methods for new rules, this is definitely needed. Please see the opened issue about it.
Currently, you can use a Json Schema Validator platform. Add only your schema content against data-schema-example.json. For testing different scenarios edit the content of the data-schema-example and make sure the rule is correct.

@dolby360
Copy link
Contributor

@OriYosef Is there a way to test the new rules? The contribution section doesn't contain info on how to perform tests on rules.

I'm working on it 😬

@jmj0502
Copy link
Contributor Author

jmj0502 commented Sep 30, 2022

@OriYosef Excellent! I'll be working on the changes. Ok, no problem. I'll use the JSON schema validator as you said! Glad somebody is working on the testing feature c: that will help a lot in the future!

…ng if the action/setup-java@v2+ is used before checking for the use of adopt/adopt-hotspot distributions.
@jmj0502
Copy link
Contributor Author

jmj0502 commented Oct 1, 2022

@OriYosef I've already made changes. Now the failureMessage of both rules provide more context about the error and how to solve it and the prevent-adopt-distributions rule now checks if actions/setup-java@v2+ is being used.

When you have some time, would you please take a look at the if condition? I still have some doubts when it comes to its use, and I would like to know if I need to make changes on that part of the rule.

"description": "Prevent use of action/setup-java@v1.",
"failureMessage": "action/setup-java@v1 is used on the workflow. action/setup-java@v1 uses a java distribution that is out of support (AdoptOpenJDK) by default, please use action/setup-java@v2+ and avoid the use of adopt/adopt-host distributions.",
"uniqueId": 12,
"enableByDefault": true,
Copy link
Contributor

Choose a reason for hiding this comment

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

typo:
enabledByDefault

please fix in all files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool! OMW

},
"then": {
"properties": {
"uses": {
Copy link
Contributor

Choose a reason for hiding this comment

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

The format of the if condition looks good. The only thing is that the distribution key is given in the with section. please see this example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okok! I'll work on the change. Should I access the contents of with using properties or additionalProperties?

Copy link
Contributor

Choose a reason for hiding this comment

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

checkout this example.

@OriYosef
Copy link
Contributor

OriYosef commented Oct 2, 2022

@jmj0502
The failureMessage looks great!
I have tested your rules and we need to make very small changes. I think we are almost done here... Kudos!

… reference to the distribution property on the rule allero-io#13.
@jmj0502
Copy link
Contributor Author

jmj0502 commented Oct 2, 2022

@OriYosef I already worked on the review comments! When it comes to the with clause, I supposed it would look like this:
image
Is that alright? I'm up to make any change if needed 👍

@OriYosef
Copy link
Contributor

OriYosef commented Oct 3, 2022

Yes, You can see the link I sent before.

@jmj0502
Copy link
Contributor Author

jmj0502 commented Oct 3, 2022

@OriYosef Cool! I reviewed the content of the link, but I wasn't sure if the with clause will translate to JSON structure similar to the one on the screen cap. I think we're done, then 🥳

Thanks for all the guidance! Definitely looking forward to keeping on making contributions.

@jmj0502 jmj0502 marked this pull request as ready for review October 3, 2022 13:03
@OriYosef
Copy link
Contributor

OriYosef commented Oct 3, 2022

@jmj0502
Yes indeed. The json file that represents the pipelines keeps the same structure.
All looks good now. I think we are done here.
Thank you for your contribution!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants