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.

Sign up for GitHub

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 defaults channel on input channel: nodefaults #364

Closed
wants to merge 5 commits into from
Closed

Remove defaults channel on input channel: nodefaults #364

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
680a72b
Remove defaults on channel nodefaults
liamhuber Aug 26, 2024
5dae362
Allow removing defaults to fail
liamhuber Aug 27, 2024
503b0a3
Refactor: rename error variable to be consistent with codebase
liamhuber Aug 27, 2024
e2848d3
Update src/conda.ts
liamhuber Sep 10, 2024
5f0df52
Build the ts committed suggestions into the js
liamhuber Sep 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions dist/setup/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47284,8 +47284,19 @@ function applyCondaConfiguration(inputs, options) {
// LIFO: reverse order to preserve higher priority as listed in the option
// .slice ensures working against a copy
for (const channel of channels.slice().reverse()) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
for (const channel of channels.slice().reverse()) {
if (channels.length) {
yield condaCommand(["config", "--remove", "channels", "defaults"], options);
}
for (const channel of channels.slice().reverse()) {

Copy link
Author

Choose a reason for hiding this comment

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

Doesn't this change the behaviour so that the absence of defaults is the new default behaviour?

I actually disagree with @oursland that including defaults by default is a bug that needs to be removed -- at least here. I'm 100% in favour of getting rid of it in the default behaviour of conda itself, but for me it's important that setup-miniconda be consistent with conda where they share terminology (like "channels"). To this end I actually don't recommend changing the default behaviour, and still never want to see the channel nodefaults explicitly get added to the channels list.

Ie fix problems upstream, and keep behaviour downstream consistent with upstream (maybe adding extra convenience on top of that consistency as in... can't find the link on mobile, but you know, the other PR relevant here)

Copy link
Member

Choose a reason for hiding this comment

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

In the case when a user specifies channels (like below), it would only use those channels

setup-miniconda/README.md

Lines 349 to 353 in 8f65dda

- uses: conda-incubator/setup-miniconda@v3
with:
activate-environment: foo
python-version: 3.6
channels: conda-forge,spyder-ide

If no channels are specified, it would fallback to the default behavior of the selected installer/Conda (typically using defaults)

Copy link
Author

Choose a reason for hiding this comment

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

But this still differs from default conda behviour, yeah? Where defaults is included by default and setting the channel nodefaults removes it rather than adding a channel nodefaults?

Copy link
Member

Choose a reason for hiding this comment

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

Not sure I follow

We don't specify how the channel public API of this GHA is implemented. It could be specified through conda config commands (like now) or it could be specified by writing out a condarc or it could be options tacked onto each conda create/conda install command or some other way not currently being considered

Think relying on how these public APIs are implemented under-the-hood is relying on an implementation detail, which would be subject to change. There is no guarantee the implementation details would remain the same

Copy link
Member

Choose a reason for hiding this comment

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

Though did miss we do say we will add defaults to the end of the list 🤦‍♂️

setup-miniconda/README.md

Lines 335 to 337 in 8f65dda

- conda-forge
- spyder-ide
- defaults

That said, I think this is worth breaking. It would mean using a major version bump

core.info(`Adding channel '${channel}'`);
yield condaCommand(["config", "--add", "channels", channel], options);
core.info("Removing channel defaults");
if (channel === "nodefaults") {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
core.info("Removing channel defaults");
if (channel === "nodefaults") {
if (channel === "nodefaults") {
core.info("Removing channel defaults");

try {
yield condaCommand(["config", "--remove", "channels", "defaults"], options);
}
catch (err) {
core.info("Removing defaults raised an error -- it was probably not present.");
}
}
else {
core.info(`Adding channel '${channel}'`);
yield condaCommand(["config", "--add", "channels", channel], options);
}
}
// All other options are just passed as their string representations
for (const [key, value] of configEntries) {
Expand Down
18 changes: 16 additions & 2 deletions src/conda.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,8 +126,22 @@ export async function applyCondaConfiguration(
// LIFO: reverse order to preserve higher priority as listed in the option
// .slice ensures working against a copy
for (const channel of channels.slice().reverse()) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
for (const channel of channels.slice().reverse()) {
if (channels.length) {
yield condaCommand(["config", "--remove", "channels", "defaults"], options);
}
for (const channel of channels.slice().reverse()) {

core.info(`Adding channel '${channel}'`);
await condaCommand(["config", "--add", "channels", channel], options);
core.info("Removing channel defaults");
if (channel === "nodefaults") {
liamhuber marked this conversation as resolved.
Show resolved Hide resolved
try {
await condaCommand(
["config", "--remove", "channels", "defaults"],
options,
);
} catch (err) {
core.info(
"Removing defaults raised an error -- it was probably not present.",
);
}
} else {
core.info(`Adding channel '${channel}'`);
await condaCommand(["config", "--add", "channels", channel], options);
}
}

// All other options are just passed as their string representations
Expand Down
Loading