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

per server validateApps #30199

Open
wants to merge 2 commits into
base: integration
Choose a base branch
from

Conversation

benjamin-confino
Copy link
Member

@benjamin-confino benjamin-confino commented Nov 15, 2024

fixes #30198

Deprecates methods on LibertyServer that make validateApps global, and introduce new methods that are perServer.

@benjamin-confino benjamin-confino force-pushed the 30198-liberty-server-app-validation branch 3 times, most recently from 437e588 to 2a9f454 Compare November 15, 2024 11:47
@benjamin-confino
Copy link
Member Author

#build

@contbld
Copy link
Collaborator

contbld commented Nov 15, 2024

Your personal pipeline request is at https://libh-proxy1.fyre.ibm.com/cognitive/pipelineAnalysis.html?uuid=60d219a0-01db-43db-b355-9f79e478847e

Target locations of links might be accessible only to IBM employees.

Comment on lines 1129 to 1138
public boolean getValidateApps() {
//orElse(false) because even if you change DEFAULT_VALIDATE_APPS that would be after
//validateAppsGlobal went through its static assignment, which used to be to the static
//value of DEFAULT_VALIDATE_APPS; which is true
return validateAppsGlobal.orElse(true);
}
Copy link
Member

Choose a reason for hiding this comment

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

DEFAULT_VALIDATE_APPS is final, so it's safe to use it here and assume it hasn't changed.

Comment on lines 1126 to 1136
//This gets the value of validateAppsGlobal, or if nothing was explicitly set it returns what would have been the value
//under the previous code where validateApps was a static variable shared across all servers
@Deprecated
public boolean getValidateApps() {
Copy link
Member

Choose a reason for hiding this comment

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

I think I would make this a simpler getter for validateAppsGlobal

Comment on lines 1117 to 1125
/*
* This method and its getter will change the validate apps setting for all servers, even ones running on other tests
* use setValidateAppsLocal to configure the setting for just the current server.
*/
@Deprecated
public static void setValidateApps(boolean validateApp) {
Copy link
Member

Choose a reason for hiding this comment

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

We should make this proper Javadoc and direct users to the correct method. Similarly for getValidateApps()

Suggested change
/*
* This method and its getter will change the validate apps setting for all servers, even ones running on other tests
* use setValidateAppsLocal to configure the setting for just the current server.
*/
@Deprecated
public static void setValidateApps(boolean validateApp) {
/**
* Sets the global flag to control whether to check that deployed apps started successfully when the server starts.
* @param validateApp the value to set the flag to
*
* @deprecated use {@link #setValidateAppsLocal(boolean)} instead
*/
@Deprecated
public static void setValidateApps(boolean validateApp) {

@benjamin-confino benjamin-confino force-pushed the 30198-liberty-server-app-validation branch from 2a9f454 to e4417e5 Compare November 15, 2024 12:13
@contbld
Copy link
Collaborator

contbld commented Nov 15, 2024

Code analysis and actions

DO NOT DELETE THIS COMMENT.
  • 1 test infrastructure code files were changed.
  • Test failures/errors in the build could be due to these changes.

@contbld
Copy link
Collaborator

contbld commented Nov 15, 2024

@benjamin-confino
Copy link
Member Author

#build

@contbld
Copy link
Collaborator

contbld commented Nov 15, 2024

Your personal pipeline request is at https://libh-proxy1.fyre.ibm.com/cognitive/pipelineAnalysis.html?uuid=1356882b-fc00-4747-a464-2984ccb79d17

Target locations of links might be accessible only to IBM employees.

@LibbyBot
Copy link

Code analysis and actions

DO NOT DELETE THIS COMMENT.
  • Please describe in a separate comment how you tested your changes.

  • 1 test infrastructure code files were changed.

  • Test failures/errors in the build could be due to these changes.

@LibbyBot
Copy link

Your personal build request is at https://wasrtc.hursley.ibm.com:9443/jazz/resource/itemOid/com.ibm.team.build.BuildResult/_xN-AgKOOEe-kGLEyqlUFUA

Target locations of links might be accessible only to IBM employees.

@LibbyBot
Copy link

Your personal build request is at https://wasrtc.hursley.ibm.com:9443/jazz/resource/itemOid/com.ibm.team.build.BuildResult/_owPXgaOUEe-kGLEyqlUFUA

Target locations of links might be accessible only to IBM employees.

@contbld
Copy link
Collaborator

contbld commented Nov 15, 2024

Code analysis and actions

DO NOT DELETE THIS COMMENT.
  • 1 test infrastructure code files were changed.
  • Test failures/errors in the build could be due to these changes.

@contbld
Copy link
Collaborator

contbld commented Nov 16, 2024

@LibbyBot
Copy link

Your personal build request is at https://wasrtc.hursley.ibm.com:9443/jazz/resource/itemOid/com.ibm.team.build.BuildResult/_XxBtoaPyEe-kGLEyqlUFUA

Target locations of links might be accessible only to IBM employees.

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

Successfully merging this pull request may close these issues.

In LibertyServer disabling app validation is per bucket rather than per server
4 participants