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

Enhance the environment variable check #5800

Merged
merged 1 commit into from
Dec 6, 2024
Merged

Conversation

sophia-guo
Copy link
Contributor

Check environment variable as set only if it's not empty.

Related #5683 (comment)

Check environment variable as set only if it's not empty.

Signed-off-by: Sophia Guo <[email protected]>
@sophia-guo
Copy link
Contributor Author

Using macros "set-property" checking both the presence of the property and whether it is non-empty to handle the case where the env.variable might be set but empty.

@sophia-guo sophia-guo requested a review from llxia December 6, 2024 15:49
@llxia
Copy link
Contributor

llxia commented Dec 6, 2024

Could you please test the change in Grinder?

@sophia-guo
Copy link
Contributor Author

Grinder shows the change won't affect current behaviour.
https://ci.adoptium.net/view/Test_grinder/job/Grinder/11989/
https://ci.adoptium.net/view/Test_grinder/job/Grinder/11992/

This change mainly handle the environment variable is set but empty, which seems can only trigger in this environment #5683 (comment). Set environment variable as empty in jenkinsfilebase https://github.com/adoptium/aqa-tests/blob/master/buildenv/jenkins/JenkinsfileBase#L83 won't trigger this issue. Fix and causes is similar to https://github.com/adoptium/TKG/pull/641/files.

Given the high cost of setting up this test environment ( trigger the dynamic agent, might also need to use George's PR) and current behaviour won't be changed I'm skipping the test the environment variable is set but empty. If any issues arise late we can address them at that time.

@llxia
Copy link
Contributor

llxia commented Dec 6, 2024

I tested openJcePlusTests in 2 scenarios (Grinder_FIPS/2896, Grinder_FIPS/2897) and they both passed. Unfortunately, I cannot test all the cases. But I think this PR is safe to merge.

Copy link
Contributor

@llxia llxia left a comment

Choose a reason for hiding this comment

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

LGTM

@llxia llxia merged commit 3752c98 into adoptium:master Dec 6, 2024
2 checks passed
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.

3 participants