8000 check java version for azure by KaiqianYang · Pull Request #957 · windup/windup-rulesets · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

check java version for azure #957

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Jun 7, 2023
Merged

check java version for azure #957

merged 5 commits into from
Jun 7, 2023

Conversation

KaiqianYang
Copy link
Contributor

No description provided.

@KaiqianYang
Copy link
Contributor Author

@PhilipCattanach Hi Philip, in this commit I encountered a issue of xmlfile match not working. I also tested many other xmlfile matching patterns but all didn't work.

Could you help me with this issue? Thank you!

@PhilipCattanach
Copy link
Contributor

Hi @KaiqianYang
I will take a look.

@PhilipCattanach
Copy link
Contributor

Created https://issues.redhat.com/browse/WINDUPRULE-995 to manage this change

KaiqianYang and others added 2 commits May 19, 2023 09:55
@KaiqianYang
Merge pull request #2 from PhilipCattanach/philipcattanach-add-java-v…
a4e11a9 
…ersion-check-for-azure

Philipcattanach add java version check for azure
use winup:matches
7668c58
@KaiqianYang
Copy link
Contributor Author
KaiqianYang commented May 19, 2023 •
edited
Loading

Created https://issues.redhat.com/browse/WINDUPRULE-995 to manage this change

Hi @PhilipCattanach , thanks for your help!

I've merged your commit into this pr. Based on your version, I made an update to further simplify the rule pattern by utilizing windup:matches(). Manual test results are now as expected.

I noticed that a key change you introduced was using /m:java.version instead of /java.version, and adding <namespace prefix="m" uri="http://maven.apache.org/POM/4.0.0"/> within <xmlfile>. Could you please provide more insight into this alteration? It seems different from standard XPath patterns.

Additionally, is including <namespace prefix="m" uri="http://maven.apache.org/POM/4.0.0"/> a requirement for the rule to function properly?

Thank you.

@PhilipCattanach
Copy link
Contributor

@KaiqianYang - thanks for the feedback. I'll take another look. I may have made a mistake.

@PhilipCattanach
Copy link
Contributor

@KaiqianYang - I'll ask one of my Engineers to get involved as their technical skills are so much better than my own.

Hi @jmle Can I ask you to review this PR please. I have tried to help but my technical skills have been found wanting.

@jmle
Copy link
Contributor
jmle commented May 25, 2023

Hey @KaiqianYang, the <xmlfile> condition will need the <namespace> element if the given XML is expected to have namespace information. If not, the <namespace> element must be removed.

Therefore, if you don't know whether the XML to analyse will have namespace information or not, the best might be to do something like this:

<or>
  <xmlfile matches="X"...>
    <namespace ...>
  </xmlfile>
  <xmlfile matches="X" .../>
</or>

This way, you will make sure that the condition is triggered regardless of whether the XML is "namespaced" or not.

@KaiqianYang
Copy link
Contributor Author

Hi @PhilipCattanach , could you help to merge this PR since it's tested successfully

@KaiqianYang
Copy link
Contributor Author

Hey @KaiqianYang, the <xmlfile> condition will need the <namespace> element if the given XML is expected to have namespace information. If not, the <namespace> element must be removed.

Therefore, if you don't know whether the XML to analyse will have namespace information or not, the best might be to do something like this:

<or>
  <xmlfile matches="X"...>
    <namespace ...>
  </xmlfile>
  <xmlfile matches="X" .../>
</or>

This way, you will make sure that the condition is triggered regardless of whether the XML is "namespaced" or not.

@jmle Thanks for your information. And I wanted to confirm that in Windup rule XML path expressions, the working pattern is //m:java.version instead of //java.version. This is different from standard XPath expressions. So, does this mean that specifying the namespace is necessary when using XML path patterns in Windup rules?

@jmle
Copy link
Contributor
jmle commented May 26, 2023

@KaiqianYang the namespace in the condition is necessary when the "target" XML has namespace information. If no namespace information is present in the XML, the namespace must be removed from the condition:

  • Target XML with namespace ➡️ condition must have namespace
  • Target XML without namespace ➡️ condition must not have namespace

Therefore, if you don't know if the XMLs you're targetting will or will not have the namespace information on them, the best you can do is to <or> two conditions as explained above.

@KaiqianYang
Copy link
Contributor Author

@KaiqianYang the namespace in the condition is necessary when the "target" XML has namespace information. If no namespace information is present in the XML, the namespace must be removed from the condition:

  • Target XML with namespace ➡️ condition must have namespace
  • Target XML without namespace ➡️ condition must not have namespace

Therefore, if you don't know if the XMLs you're targetting will or will not have the namespace information on them, the best you can do is to <or> two conditions as explained above.

Yes, your response is quite clear. Thank you.

jmle
jmle reviewed May 26, 2023
View reviewed changes
rules/rules-reviewed/azure/azure-java-version.windup.xml
<rule id="azure-java-version-01000">
<when>
<xmlfile matches="//m:java.version[windup:matches(text(), '{v}')]" in="pom.xml" as="result">
<namespace prefix="m" uri="http://maven.apache.org/POM/4.0.0"/>
Copy link
Contributor
@jmle jmle May 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Continuing with my comments, in this case, this 8000 is correct, since the POM files in the test folders have namespace information, although it is possible to find POM files without namespace information that build correctly. If you want to capture those edge cases, you will have to <or> this condition with an additional one without the namespace element.

@jmle
Copy link
Contributor
jmle commented Jun 6, 2023

@KaiqianYang I was taking a look at this again. After my clarification, is there something you want to change or are you waiting for it to get merged?

@KaiqianYang
Copy link
Contributor Author

@KaiqianYang I was taking a look at this again. After my clarification, is there something you want to change or are you waiting for it to get merged?

Hi @jmle , I think it's fine now. Waiting for it to get merged.
Thank you.

@jmle jmle merged commit be700f2 into windup:master Jun 7, 2023
mrizzi added a commit that referenced this pull request Jun 8, 2023
@agoncal @carlosthe19916
@KaiqianYang @PhilipCattanach @mrizzi
Overriding rules (#987)
76c8e1b 
* Lombok <1.18.22 is not compatible with JDK 17

* Overriding rules

* [WINDUPRULE-1005]-JWS: Upgrade direct dependencies that belong to the groupId org.apache.tomcat (#972)

* Add tomcat target and dependencies rule

* Add Tomcat direct dependency check rules

* Change toVersion

* check java version for azure (#957)

* check java version for azure

* Azure java version identification

* use winup:matches

---------

Co-authored-by: kaiqianyang <kaiqianyang@microsoftcom>
Co-authored-by: PhilipCattanach <pcattana@redhat.com>

* Enabled 'rules-overridden-azure' testing on GH

---------

Co-authored-by: Carlos E. Feria Vila <carlosthe19916@gmail.com>
Co-authored-by: KaiqianYang <89442934+KaiqianYang@users.noreply.github.com>
Co-authored-by: kaiqianyang <kaiqianyang@microsoftcom>
Co-authored-by: PhilipCattanach <pcattana@redhat.com>
Co-authored-by: mrizzi <mrizzi@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmle jmle jmle left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@KaiqianYang @PhilipCattanach @jmle
0