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

Remove 'name' field as mandatory from environment_schema.json #143

Merged
merged 2 commits into from
Sep 9, 2024

Conversation

rnaidu
Copy link
Collaborator

@rnaidu rnaidu commented Sep 6, 2024

PR checklist

Closes #142

  • Removing 'name' field in environment_schema.json since updated version of nf-core tools released today is throwing an error if provided in user's environment.yml.
  • Removing 'defaults' channel field in environment_schema.json since updated version of nf-core tools released today is throwing an error if provided in user's environment.yml.
  • Inserting glob pattern under 'dependencies' field to match what is required by nf-core lint check

@rnaidu rnaidu changed the title Merge pull request #134 from mskcc-omics-workflows/develop Remove 'name' field as mandatory from environment_schema.json Sep 6, 2024
@rnaidu rnaidu requested a review from a team as a code owner September 6, 2024 20:46
@rnaidu rnaidu added the bug Something isn't working label Sep 6, 2024
@rnaidu rnaidu self-assigned this Sep 6, 2024
@rnaidu rnaidu requested review from buehlere and johnoooh September 6, 2024 20:51
Copy link
Collaborator

@buehlere buehlere left a comment

Choose a reason for hiding this comment

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

I'm thinking about this in two ways.

Either we just merge in the schema change and let people fix their individual modules when they see the lint failing. Or we could just go ahead and remove the name field all modules.

I think both should be pretty easy. I'm leaning towards just removing it for all modules.

I also noticed defaults is being removed is that part of the linting update?

@rnaidu
Copy link
Collaborator Author

rnaidu commented Sep 9, 2024

I'm thinking about this in two ways.

Either we just merge in the schema change and let people fix their individual modules when they see the lint failing. Or we could just go ahead and remove the name field all modules.

I think both should be pretty easy. I'm leaning towards just removing it for all modules.

I also noticed defaults is being removed is that part of the linting update?

@buehlere I agree on removing 'name' for all modules, since the conversation on the nf-core slack said the 'name' field will be removed from the new module template moving forward. I'll add that change now

I think removing 'defaults' is part of the linting update as well since I updated my nf-core and used this command to create a module off the updated nf-core modules template. The new module created by the updated template has both 'name' and 'defaults' removed. I reached out to the nf-core maintainer in that slack thread in case we want a confirmation from them before merging the schema.

nf-core modules create testing --author @rnaidu --label process_low --meta

@rnaidu rnaidu force-pushed the feature/remove_name_from_schema branch 2 times, most recently from e6dfbfa to bf219c9 Compare September 9, 2024 19:21
…nting update. Users will have to edit current module environment.yml files accordingly. future modules downloaded by nf-core will follow configuration set in this environment_schema.json
@rnaidu
Copy link
Collaborator Author

rnaidu commented Sep 9, 2024

Final environment_schema.json includes:

  • removal of 'name' field
  • removal of 'defaults' field under channels.
  • addition of glob pattern set by nf-core lint under 'dependencies'

@rnaidu rnaidu requested a review from buehlere September 9, 2024 19:28
Copy link
Collaborator

@buehlere buehlere left a comment

Choose a reason for hiding this comment

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

looks good to me!

@rnaidu rnaidu merged commit d5d61c0 into develop Sep 9, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Will need to update environment_schema.json to make 'name' field not mandatory
2 participants