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

Unset CUSTOMIZED_SDK_URL if SDK_RESOURCE is not customized #5034

Merged
merged 2 commits into from
Feb 8, 2024

Conversation

sophia-guo
Copy link
Contributor

CUSTOMIZED_SDK_URL is secondary parameter determined by primary parameter SDK_RESOURE. If SDK_RESOURCE is set as non-customized, CUSTOMIZED_SDK_URL should be set as none( except nightly)

Fix #2097

CUSTOMIZED_SDK_URL is secondary parameter determined by primary
parameter SDK_RESOURE. If SDK_RESOURCE is set as non-customized,
CUSTOMIZED_SDK_URL should be set as none( except nightly)

Signed-off-by: Sophia Guo <sophia.gwf@gmail.com>
@sophia-guo sophia-guo requested a review from smlambert February 2, 2024 17:45
@smlambert smlambert requested a review from llxia February 3, 2024 01:50
@smlambert
Copy link
Contributor

Asking for a review from @llxia to check it works for the ways that this field may be overwritten to pickup nightly openj9 builds and when used as part of AQA_Test_Pipeline.

@sophia-guo
Copy link
Contributor Author

I believe that nightly openj9 builds is in the category if (params.SDK_RESOURCE == 'nightly' && params.CUSTOMIZED_SDK_URL) , which won't be affected.

@@ -401,6 +401,8 @@ def setup() {
if (params.SDK_RESOURCE == 'nightly' && params.CUSTOMIZED_SDK_URL) {
// remove single quote to allow variables to be set in CUSTOMIZED_SDK_URL
CUSTOMIZED_SDK_URL_OPTION = "-c ${params.CUSTOMIZED_SDK_URL}"
} else if (params.SDK_RESOURCE != 'customized') {
CUSTOMIZED_SDK_URL_OPTION = ""
Copy link
Contributor

@llxia llxia Feb 5, 2024

Choose a reason for hiding this comment

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

Here, we assume if user provides SDK_RESOURCE != 'customized' and CUSTOMIZED_SDK_URL, we will ignore CUSTOMIZED_SDK_URL value and proceed with SDK_RESOURCE value only.
IMO, we should just fail in this case.

I would like to rewrite the whole logic to be more readable:

if (params.CUSTOMIZED_SDK_URL) {
	if (params.SDK_RESOURCE == 'nightly') {
		// remove single quote to allow variables to be set in CUSTOMIZED_SDK_URL
        	CUSTOMIZED_SDK_URL_OPTION = "-c ${params.CUSTOMIZED_SDK_URL}"
	} else if (!params.SDK_RESOURCE || params.SDK_RESOURCE == 'customized') {
		SDK_RESOURCE = "customized"
		CUSTOMIZED_SDK_URL_OPTION = "-c '${params.CUSTOMIZED_SDK_URL}'"
                … // RI_JDK code
        } else {
		error("SDK_RESOURCE: ${params.SDK_RESOURCE} and CUSTOMIZED_SDK_URL: ${params.CUSTOMIZED_SDK_URL} combo is not supported!)
        }
} else {
        if (params.SDK_RESOURCE == 'customized') {
		error("SDK_RESOURCE: ${params.SDK_RESOURCE}, please provide CUSTOMIZED_SDK_URL")
        }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a more strict user story. Updated.

@llxia
Copy link
Contributor

llxia commented Feb 7, 2024

Thanks @sophia-guo . Could you run some Grinder to verify?

@sophia-guo sophia-guo force-pushed the reproD branch 2 times, most recently from c75173f to b98b7ae Compare February 7, 2024 15:19
Fail for all other combo

Signed-off-by: Sophia Guo <sophia.gwf@gmail.com>
@llxia
Copy link
Contributor

llxia commented Feb 8, 2024

Thanks @sophia-guo .
I have also tested the case of the SDK_RESOURCE=nightly and CUSTOMIZED_SDK_URL for openj9 (internal link)

@llxia llxia merged commit 09d681e into adoptium:master Feb 8, 2024
1 check 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.

Setting CUSTOMIZED_SDK_URL and SDK_RESOURCE as non-customized ignores the SDK_RESOURCE value
4 participants