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

Myjobs: Add ENV override for showing job array #2327

Merged
merged 1 commit into from
Oct 27, 2022

Conversation

covert8
Copy link
Contributor

@covert8 covert8 commented Oct 12, 2022

Hi,

We are exploring ondemand at Kuleuven and we would like to not show the job array in the myjobs app due to a competing implementation.

This pr would add an environment variable acting as an override for showing the job array.
Is this the best way to go about it?

Thank you in advance.

┆Issue is synchronized with this Asana task by Unito

@covert8 covert8 changed the title Myjob: Add ENV override for showing job array Myjobs: Add ENV override for showing job array Oct 12, 2022
@johrstrom
Copy link
Contributor

Hi, thank you for your contribution!

While what you have is OK - It's probably better to put this into the configuration_singleton just so all configurations are co-located in one file.

Here's an example (also using a to_bool helper):

def show_job_options_account_field?
to_bool(ENV.fetch('OOD_SHOW_JOB_OPTIONS_ACCOUNT_FIELD', true))
end

  def self.show_job_arrays?
    return false if Configuration.show_job_arrays?

    OODClusters.any? { |cluster| cluster.job_adapter.supports_job_arrays? }
  end

@covert8 covert8 force-pushed the fix-show-job-arrays branch from b42cd8e to b0203b9 Compare October 12, 2022 14:24
@covert8
Copy link
Contributor Author

covert8 commented Oct 12, 2022

Thank you for the tips and quick response

Due to the to_bool behavior an empty string (unset variable) would lead to a false causing the default behavior to be not showing the job array; i changed the variable to be a hide, to make sure the default behavior would be to show the job array.

Also, where in the docs are the variables kept so I can update it?

@covert8 covert8 force-pushed the fix-show-job-arrays branch from b0203b9 to 560dd68 Compare October 12, 2022 14:50
@johrstrom
Copy link
Contributor

@covert8
Copy link
Contributor Author

covert8 commented Oct 18, 2022

Hi, just for confirmation. Are there any more changes required to merge this commit?

@johrstrom
Copy link
Contributor

A thousand apologies for not updating you - I was at a conference last week so I couldn't get to it.

I don't believe there are updates I require, I just need to find the time to look it over a second time. I should be able to get it today or tomorrow.

@covert8 covert8 force-pushed the fix-show-job-arrays branch from 560dd68 to 3096507 Compare October 27, 2022 13:17
covert8 added a commit to covert8/ood-documentation that referenced this pull request Oct 27, 2022
Copy link
Contributor

@johrstrom johrstrom left a comment

Choose a reason for hiding this comment

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

Thanks again for the contribution. Once the tests pass, I'll merge this. I'll also backport it too because we're releasing 2.0.29 out soon.

@johrstrom johrstrom merged commit 6a83a94 into OSC:master Oct 27, 2022
johrstrom added a commit that referenced this pull request Oct 31, 2022
johrstrom pushed a commit to OSC/ood-documentation that referenced this pull request Nov 17, 2022
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.

3 participants