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

DM-42530: changes to templates for START condition etc #10

Merged
merged 1 commit into from Feb 17, 2024

Conversation

daues
Copy link
Contributor

@daues daues commented Jan 26, 2024

No description provided.

@MichelleGower MichelleGower changed the title changes to templates for START condition etc DM-42530: changes to templates for START condition etc Jan 30, 2024
Copy link

@MichelleGower MichelleGower left a comment

Choose a reason for hiding this comment

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

A comment about leaving commented out values. Merge approved.

@@ -27,7 +27,7 @@ AUTO_INCLUDE_SHARED_PORT_IN_DAEMON_LIST=False
USE_SHARED_PORT=False

# START = ( (Owner == "$USER_NAME") && (JOB_NODE_SET == "$NODE_SET" ) )
START = (Owner == "$USER_NAME")
# START = (Owner == "$USER_NAME")

Choose a reason for hiding this comment

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

If not providing examples for something a user can set in a config, we probably should not have commented out code when branch is ready to merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes the comment lingering there is kind of cryptic reminder that "we used to set this here, but now it is set elsewhere, in env vars" (in case someone would actually look for the START expression).
So yes it can be removed, or maybe the comment statement can actually just say that the START condition is not set in the config but in scripted Env vars.

@daues daues merged commit e038d2a into main Feb 17, 2024
2 checks passed
@daues daues deleted the tickets/DM-42530 branch February 17, 2024 00:11
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.

None yet

2 participants