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

PR: Allow null channels in file to not crash setup. #120

Merged
merged 1 commit into from
Dec 24, 2020

Conversation

Lnaden
Copy link
Contributor

@Lnaden Lnaden commented Dec 11, 2020

There is an edge case where having channels in an environment file
but an empty list, and channels missing from the GHA options/keys
returns an unhelpful error message:

Error: Cannot read property 'join' of null

I've setup an example here which runs through all the permutations of
channels keys being set, missing, and nulled:

https://github.com/Lnaden/ghatesting/runs/1538195898?check_suite_focus=true

This PR attempts to fix that by relaxing the strict equals in setup.ts
from

if (condaConfig["channels"] === "" && channels !== undefined) {

to

if (condaConfig["channels"] === "" && channels != null) {

which should allow channels != null to catch both null and
undefined for channels. I also attempted to add an example to the
GitHub Actions tests (Example 9) to test this edge case specifically.

A few comments to this PR:

  • I'm not familiar with Typescript and building GHAs, so this might not be correct and I'm happy to make changes and take feedback.
  • If you use an env file with an empty channels list with the conda client to create an environment, it works fine and just defaults to whatever the conda is configured to. In my example above, setting no channels in either the file or the GHA keys works fine and pulls from the default channel.
  • This behavior admittedly is ambiguous, so if you would rather this edge case throw an error, I would like to request a better error message.
  • The package.json and package-lock.json files may not need to be merged with this? I followed the instructions on the CONTRIBUTING.md file and these wound up getting changed. Is that expected (e.g. 11k lines on package-lock)?

@goanpeca goanpeca requested a review from bollwyvl December 19, 2020 23:02
@goanpeca
Copy link
Member

@Lnaden thanks a lot for working on this fix :)

I updated your PR and rebased with latest master to solve the conflicts.

src/setup.ts Outdated
if (condaConfig["channels"] === "" && channels !== undefined) {
// Check if channels is undefined or null (empty list) in the file.
// != should catch both, !== will get only one.
if (condaConfig["channels"] === "" && channels != null) {
Copy link
Member

Choose a reason for hiding this comment

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

Any thoughts @bollwyvl :) ?

@bollwyvl
Copy link
Contributor

I'm pretty sure this is now covered by:

https://github.com/conda-incubator/setup-miniconda/blob/master/src/conda.ts#L97

  let channels = inputs.condaConfig.channels /* plus some string stuff */

  if (!channels.length && options.envSpec?.yaml?.channels?.length) {
    channels = options.envSpec.yaml.channels;
  }

@goanpeca goanpeca self-assigned this Dec 24, 2020
@goanpeca goanpeca closed this Dec 24, 2020
@goanpeca
Copy link
Member

I will update to just use the sample yaml.

@goanpeca goanpeca reopened this Dec 24, 2020
@goanpeca goanpeca merged commit 38142d1 into conda-incubator:master Dec 24, 2020
@Lnaden
Copy link
Contributor Author

Lnaden commented Dec 24, 2020

Awesome, glad this got resolved regardless of how.

@Lnaden Lnaden deleted the fix-empty-channels branch December 24, 2020 19:53
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