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

Add customizations for use in HEB 115 #3

Merged
merged 8 commits into from
Sep 23, 2024
Merged

Add customizations for use in HEB 115 #3

merged 8 commits into from
Sep 23, 2024

Conversation

jaguillette
Copy link
Contributor

@jaguillette jaguillette commented Sep 12, 2024

This PR sets up the spack app with a custom version just for HEB 115, which is in fact the only version available. It sets a pattern for making apps available only to certain classes, and for setting up sub-apps with slightly different configurations.

Sub-App Definition

The sub-app is configured in local/humanEvolutionaryBiology-115.yml.erb. This setup is described in the OOD docs on sub-apps. I initially thought that the only way to change how the app shows up is to change the name of the yml file, but Issue 3005 in the OOD repo indicates that sub-apps can now override the metadata of their parent app. That means that we can override the title of the app with a top-level title: attribute in the form.yml variant that sets up the sub-app.

In this case, the sub-app hard codes the Spack installation and environment to use. There is an existing Spack environment for HEB 115, and I've added Jupyter Lab and the usual suspects for Python to that environment, so that the tools configured for the course are available by default. The definition for this environment has been added to the spack environments folder.

Restricted Access

The other part of that yml.erb file is the determination of who to show the app to. This is done with a Ruby function available in OnDemand to grab group names and then some chained commands to turn that into a list of Canvas IDs for courses that the user is enrolled in. This is compared to a list of Canvas IDs that for courses the app is supposed to be enabled for. If there's overlap between the two lists, then the app is displayed. If there isn't then it isn't. The way this visibility is changed is by setting the cluster attribute on the form. If it's set to "*", then the app will be displayed, as it is enabled for any cluster available. To disable the app, cluster is set to disable-this-app. Since there is no cluster by that name, the app is not displayed. The cluster attribute is set based on the Ruby functions described.

Future Enhancement

An enhancement for the app would be to reference custom metadata attached to the cluster definition file. As written, if an app configured this way needs to be made available to a different class, then the app configuration must be changed on the portal node by pulling from GitHub. This isn't too hard in our current setup, with one portal node. However, with multiple portal nodes, we would need a mechanism to update the app configurations on all of the running portal nodes. Instead, we could hook into the existing cluster config update method, ensuring that changes propagate once the cluster definition file in S3 is updated.

@jaguillette jaguillette marked this pull request as draft September 12, 2024 15:40
@jaguillette jaguillette marked this pull request as ready for review September 12, 2024 20:27
userGroups = OodSupport::User.new.groups.sort_by(&:id).map(&:name).flat_map{ |str| str.scan(/^canvas(\d+)-\d+/) }.flatten
enabledGroups = [
"135510", # HUIT Open OnDemand Testing
"139860" # HEB 115: Investigating the Human Genome
Copy link

Choose a reason for hiding this comment

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

Nice and clear pattern for adding sub apps to other courses!

Copy link
Member

Choose a reason for hiding this comment

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

Agreed! This is a nice pattern that we can use for other sub-apps as well. Do we think that it's clear from the context that enabledGroups and userGroups refer to lists of Canvas Course IDs, not POSIX group names? I'm mainly asking because as we propagate these changes to other apps, it may be helpful to quickly locate places that are operating specifically on Canvas Course IDs (like here), compared to places that are operating on POSIX group names (or IDs).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe a note is warranted, since there's some nuance to it. It's the Canvas Course IDs, which are used because they're present in the grouper group names which are translated into POSIX group names.

Copy link

@Tanvez Tanvez left a comment

Choose a reason for hiding this comment

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

Awesome work @jaguillette 🚀 Really cool to see sub apps used this way!

@jaguillette
Copy link
Contributor Author

Note that I've pushed this out in prod, but haven't set up a PR to add it to IAC yet, pending approval on this PR. Just wanted to get the functionality in front of the instructor.

Copy link
Member

@arthurian arthurian left a comment

Choose a reason for hiding this comment

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

The sub-app approach that you outlined and implemented @jaguillette looks like a good way to go for customizing apps per-course. Thanks for the detailed description, including the caveat about managing app configurations on multiple portal. We'll have to remember that as we move forward with that. Great work! 👍

# @note Used to define the submitted cluster, title, description, and
# hard-coded/user-defined attributes that make up this Batch Connect app.
<%-
userGroups = OodSupport::User.new.groups.sort_by(&:id).map(&:name).flat_map{ |str| str.scan(/^canvas(\d+)-\d+/) }.flatten
Copy link
Member

@arthurian arthurian Sep 17, 2024

Choose a reason for hiding this comment

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

Great use of OodSupport to get the user groups (I wasn't previously aware of this helper library). One thing that jumps out at me as being slightly unclear, and this may just be my lack of familiarity with Ruby, is the use of flat_map. I wonder if that could be replaced with a standard map and then just flatten once at the end. To me that reads a little clearer and maybe less redundant, although the result will be exactly the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no strong opinions about Ruby syntax, so I'm happy to change it if it makes things clearer. Any Ruby code I'm writing, I'm doing with significant help from Stack Overflow and GPT.

userGroups = OodSupport::User.new.groups.sort_by(&:id).map(&:name).flat_map{ |str| str.scan(/^canvas(\d+)-\d+/) }.flatten
enabledGroups = [
"135510", # HUIT Open OnDemand Testing
"139860" # HEB 115: Investigating the Human Genome
Copy link
Member

Choose a reason for hiding this comment

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

Agreed! This is a nice pattern that we can use for other sub-apps as well. Do we think that it's clear from the context that enabledGroups and userGroups refer to lists of Canvas Course IDs, not POSIX group names? I'm mainly asking because as we propagate these changes to other apps, it may be helpful to quickly locate places that are operating specifically on Canvas Course IDs (like here), compared to places that are operating on POSIX group names (or IDs).

@jaguillette jaguillette merged commit d0b28d5 into main Sep 23, 2024
@jaguillette jaguillette deleted the heb115 branch September 23, 2024 20:14
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