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

Finish/fix SKIP_JCK_GIT_UPDATE variable in JCK #5740

Merged
merged 4 commits into from
Nov 15, 2024

Conversation

judovana
Copy link
Contributor

@judovana judovana commented Nov 11, 2024

It seems the SKIP_JCK_GIT_UPDATE was not finished/was single usage targeted/or was broken in later commits.

Before this PR, the JCK_GIT_REPO was mandatory to exists even if the SKIP_JCK_GIT_UPDATE was on place. In addition, the JCK_ROOT (which was supposed to be not updated) was deleted if the checks failed.

This PR is extending the usage of SKIP_JCK_GIT_UPDATE so the JCK_GIT_REPO is nto used at all and JCK_ROOT do not need to pass the git checs ( so it do not need to be repo art all)

Small side note - to fake git repo, which contains unpacked compatibility jars, takes some 30 minutes and used pretty quite a lot of disk space. This PR is making this creation of fake repository redundant.

#5745

@llxia
Copy link
Contributor

llxia commented Nov 12, 2024

I think SKIP_JCK_GIT_UPDATE was introduced via #2025. @karianna does SKIP_JCK_GIT_UPDATE still get used in AzDo?

@judovana
Copy link
Contributor Author

Well if it is not used in AzDo, I would like to use it. Because I have tck test-jars stored in binary, and distributed without repository. So I can prepare JCK_ROOT directly, and stop transforming it to repository which is quite expensive operation.

jck/build.xml Outdated
<not>
<equals arg1="${return.code}" arg2="0"/>
</not>
<isset property="env.SKIP_JCK_GIT_UPDATE" />
Copy link
Contributor

@sophia-guo sophia-guo Nov 12, 2024

Choose a reason for hiding this comment

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

Since SKIP_JCK_GIT_UPDATE is a boolean, the check will always return true as long as it is set. Maybe check to assess the specific value of env.SKIP_JCK_GIT_UPDATE instead of checking if it's set.

<equal arg1="{env.SKIP_JCK_GIT_UPDATE}" arg2="true">

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original code was just checking the "is set" . I have absolutely no objections to change it to explicit value. TY!

@sophia-guo
Copy link
Contributor

I opened an issue in temurin-compliance. More discussion will be moved there

@judovana
Copy link
Contributor Author

ty! Will follow

@judovana
Copy link
Contributor Author

@karianna Sophia (@sophia-guo )is in favour to change the SKIP_JCK_GIT_UPDATE isSet to SKIP_JCK_GIT_UPDATE==true that will break AzDo. Do you have any more info or anybody to ping?

@smlambert
Copy link
Contributor

We will assume that the AzDo use case is very similar to RH QE use case where 1) they are not using our Jenkinsfile code and 2) the TCK materials are housed somewhere of their choosing and they will manage the update of them outside of this framework.

Tagging @d3r3kk for awareness

@judovana judovana force-pushed the skippableJckGitRepo branch from ac9fe9a to aae8145 Compare November 14, 2024 10:51
@judovana judovana force-pushed the skippableJckGitRepo branch from aae8145 to d1a9c6f Compare November 14, 2024 11:11
@judovana
Copy link
Contributor Author

Tahnx all, changed to check for explicit "true".

@sophia-guo
Copy link
Contributor

Temurin-compliance grinder/2632,4634,4635 work as expected.

@smlambert smlambert removed the request for review from llxia November 15, 2024 00:33
@smlambert smlambert merged commit 58a7db0 into adoptium:master Nov 15, 2024
2 checks passed
@judovana
Copy link
Contributor Author

tyvm!

sophia-guo pushed a commit to sophia-guo/openjdk-tests that referenced this pull request Nov 18, 2024
* Do not enforce JCK_GIT_REPO if SKIP_JCK_GIT_UPDATE is set

* Skipping all git checks when SKIP_JCK_GIT_UPDATE is set

* Adding SKIP_JCK_GIT_UPDATE to readme

* Updated usage of SKIP_JCK_GIT_UPDATE so it needs to be true, not just set
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.

4 participants