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

Require conda <23.1.0 #627

Merged
merged 4 commits into from
Jan 25, 2023
Merged

Require conda <23.1.0 #627

merged 4 commits into from
Jan 25, 2023

Conversation

dbast
Copy link
Member

@dbast dbast commented Jan 24, 2023

attempt to workaround until there is a real fix:

  File "/usr/share/miniconda/envs/constructor/lib/python3.10/site-packages/constructor/conda_interface.py", line 143, in write_repodata
    url = used_repodata.pop('_url').rstrip("/")
KeyError: '_url'

see in https://github.com/conda/constructor/actions/runs/3998908201/jobs/6862143485

Description

Checklist - did you ...

  • Add a file to the news directory (using the template) for the next release's release notes?
  • Add / update necessary tests?
  • Add / update outdated documentation?

@dbast dbast requested a review from a team as a code owner January 24, 2023 18:07
@conda-bot conda-bot added the cla-signed [bot] added once the contributor has signed the CLA label Jan 24, 2023
@dholth
Copy link

dholth commented Jan 24, 2023

We began to store _url in a separate file, instead of in cached repodata.json

We would like to provide a real cache API. What should it look like?

@AndrewVallette
Copy link
Contributor

This looks fine to me for now.

@dbast
Copy link
Member Author

dbast commented Jan 25, 2023

Currently constructor creates its on repodata to ship it with the installer, see https://github.com/conda/constructor/blob/main/constructor/conda_interface.py#L109

I see two options to move forward:

  • conda provides a public API to access the repop data (access to a private field is actually what broke here)
  • or the entire constructor def write_repodata(...) function is replaced by a conda-index call against a local folder with all the packages that are shipped by the installer

Both options require some development ... calling conda-index for me would be a better architecture and separation of concerns and it would also better provide compatibility with both old and newer conda versions.

@dholth What do you think?

Will create an issue to track that topic.

Can we till then merge this PR in order get the CI working again. @jezdez @conda/constructor

@dbast
Copy link
Member Author

dbast commented Jan 25, 2023

Created #628 to track this after the PR merge.

@jaimergp
Copy link
Contributor

Thanks @dbast

This LGTM. We also need to patch repodata.

Copy link
Contributor

@jaimergp jaimergp left a comment

Choose a reason for hiding this comment

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

Please add news too.

@dbast dbast merged commit 5882702 into conda:main Jan 25, 2023
@dbast dbast deleted the pin_conda branch January 25, 2023 09:59
@dbast
Copy link
Member Author

dbast commented Jan 25, 2023

This was auto-merge in action ... the changes built green before... so no worries

@jaimergp
Copy link
Contributor

It was spooky! I thought auto-merge had to wait for all-green 😂

@dbast
Copy link
Member Author

dbast commented Jan 25, 2023

The list of required checks was empty, updated it to:

checks

@dbast
Copy link
Member Author

dbast commented Jan 25, 2023

@jaimergp Happy to further adjust those settings... a pity that those settings cannot be changed via a PR to file in the repo :/

@dbast
Copy link
Member Author

dbast commented Jan 25, 2023

@jaimergp introducing e.g. https://github.com/repository-settings/app would be awesome ... or managing all conda org repo settings via terraform in the conda/infra repo (that is/was afaik explored/planed).

@jezdez
Copy link
Member

jezdez commented Jan 25, 2023

@jaimergp introducing e.g. https://github.com/repository-settings/app would be awesome ... or managing all conda org repo settings via terraform in the conda/infra repo (that is/was afaik explored/planed).

This is a discussion better reserved for the infra repo, also the conversation of repo settings, to make sure we don't lose sight of it. In short: it has been considered and will be picked up again once the last legal details are resolved. @dbast please make sure to not abuse your current owner privileges, by changing repo settings without record keeping.

@dholth
Copy link

dholth commented Jan 25, 2023

@dbast yes, if we could use conda-index that would make much more sense compared to, I'm not sure of the constructor details, faking a remote channel that will immediately be overwritten when conda accesses the network. https://github.com/conda/conda-index/blob/main/conda_index/cli/__init__.py shows how to invoke conda-index (not the api module).

@dbast
Copy link
Member Author

dbast commented Jan 25, 2023

@jezdez No worries, please read above. Here some more details

  • The auto-merge feature is afaik enabled since before I started to work with this repo. @jaimergp asked me some days back and I confirmed it was already activated and it requires "Status checks" to pass
  • I today pressed the auto-merge button on this PR
  • @jaimergp approved the PR while CI was still running and it was immediately merged (with unfinished PR runs)
  • This was unexpected for both him and me
  • I investigated and found out that the list of required status checks was empty and that this lead to this unexpected behaviour
  • In order to prevent this unexpected behaviour, I used my $owner head (knowing the required status checks in other repos of the conda org) and added some status checks while making transparent with a screenshot above to show what I actually did
  • I also made myself available by stating "Happy to further adjust those settings" (I watch the repo / my Github mails / Matrix) in order to assist if somebody is unhappy about those immediate actions.
  • I also stated that I am unhappy feeling forced to do such step due to the settings not available as file for changes via PRs (imho a big GitHub flaw, that other platforms implemented different).
  • I also told that this is a topic for the conda/infra repo
  • It was and is very important to me to provide such information at the place where everything started to happen (this PR).

tldr; No code was merged to main without review (just unfinished CI of a tiny PR) and the added status checks shouldn't be a big blocker (if it is, then I am there to change the settings / revert as stated above).

@jezdez I searched, but didn't find any issue where the terraform settings topic or e.g. https://github.com/repository-settings/app have been discussed? Do you have a link for that?

@jezdez
Copy link
Member

jezdez commented Jan 25, 2023

@dbast I appreciate the details, just noting that if I hadn't seen your comments accidentally this would have been lost in GitHub notification nirvana :D This is obviously a side effect of the repo maintenance processes being still.. in flux.

And to be super clear: No hard feelings, and instead my apologies that this wasn't correctly set up and you two had a moment of unexpected behavior. In doubt, I just want to prevent us from abusing our owner privileges while we have public methods instead. Just file an infra ticket. You're right that the better solution would be to have a terraform setup in the infra repo (I have the code locally) and just submit a PR.

FWIW the terraform/repo settings work predates the infra repo (which was actually a result of that) and only has an Anaconda internal Jira ticket IIRC ("something something review GitHub repo and organization" IIRC).

@github-actions github-actions bot added the locked [bot] locked due to inactivity label Jan 26, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla-signed [bot] added once the contributor has signed the CLA locked [bot] locked due to inactivity
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants