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

Support hash-only build paths #777

Merged

Conversation

nkaretnikov
Copy link
Contributor

@nkaretnikov nkaretnikov commented Mar 11, 2024

Fixes #678.

Description

This pull request:

  • adds hash-only build paths (v3)
  • updates a test
  • updates documentation.

Pull request checklist

  • Did you test this change locally?
  • Did you update the documentation (if required)?
  • Did you add/update relevant tests for this change (if required)?

Additional information

How to test

Automated testing:

See tests changed in this PR.

Manual testing:

  • with ~/.conda-store containing builds created on main, switch to this branch and create a new build: old and new builds should be accessible in the UI (lockfile, archive, and log links should work; marking one of the old builds as default should work)
  • same, but also explicitly set build_key_version = 3 in the config
  • same, but also explicitly set build_key_version = 2 in the config (make sure the previous default works with this PR)
  • make sure two exact same specifications can be created in different namespaces and one of them is preserved when the other one is deleted.

Copy link

netlify bot commented Mar 11, 2024

Deploy Preview for conda-store ready!

Name Link
🔨 Latest commit 806da5f
🔍 Latest deploy log https://app.netlify.com/sites/conda-store/deploys/66319c616dfacf0008a939a3
😎 Deploy Preview https://deploy-preview-777--conda-store.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@nkaretnikov nkaretnikov added needs: discussion 💬 This item needs team-level discussion before scoping status: needs changes 🧱 labels Mar 11, 2024
@nkaretnikov

This comment was marked as outdated.

@nkaretnikov
Copy link
Contributor Author

nkaretnikov commented Mar 17, 2024

@jaimergp This is tested and ready for review, PTAL. Ignore the CI error, it's unrelated.

@nkaretnikov nkaretnikov added needs: review 👀 and removed needs: discussion 💬 This item needs team-level discussion before scoping status: needs changes 🧱 labels Mar 17, 2024
@nkaretnikov nkaretnikov marked this pull request as ready for review March 17, 2024 04:57
Co-authored-by: jaimergp <jaimergp@users.noreply.github.com>
@nkaretnikov nkaretnikov added status: needs changes 🧱 needs: discussion 💬 This item needs team-level discussion before scoping and removed needs: review 👀 labels Mar 21, 2024
@jaimergp
Copy link
Member

After the discussion in today's weekly:

Some background:

  • I am concerned with adding more and more versions to the path settings. That's why I was pushing on "solving this once and for all". But that's just an opinion. If the team is ok with having several path versions, then my asks in items below are not so important.

My asks:

  • Build IDs used to identify each attempt in the database shouldn't be so tied to the final path. Instead, we should resolve the lockfile first, and then compute the filesystem path from that output.
  • Since that will involve more work, the current compromise of build-id + timestamp + username is ok. I do insist that this should just be an internal identifier in the database. The filesystem path could be computed later once input data is available.

As things stand, this is a fixed-length implementation of v2 right? We could try a hashed-lockfile in a future v4.

@nkaretnikov

This comment was marked as outdated.

@nkaretnikov nkaretnikov added status: needs changes 🧱 and removed needs: discussion 💬 This item needs team-level discussion before scoping labels Mar 29, 2024
@jaimergp
Copy link
Member

jaimergp commented Apr 2, 2024

I talked with @trallard and we agree that we are good to merge, with the following reservations:

  • Mark v3 as experimental (i.e. call it v3_experimental) with no guarantees of hash stability across conda-store versions (for now).
  • Document that this is going to receive iterations where we experiment with different ingredients for the hash. In the future we might want to revisit the lockfile hashing until we arrive to a more robust solution.
  • Add a little section to the https://conda.store/community/policies/backwards-compatibility docs.

How does that sound? Thanks!

@nkaretnikov
Copy link
Contributor Author

@jaimergp PTAL. Will rebase after this is approved.

@nkaretnikov nkaretnikov merged commit cc2f545 into conda-incubator:main May 1, 2024
27 checks passed
peytondmurray pushed a commit to peytondmurray/conda-store that referenced this pull request May 4, 2024
Co-authored-by: jaimergp <jaimergp@users.noreply.github.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done 💪🏾
Development

Successfully merging this pull request may close these issues.

[ENH] - Support hash-only build paths
2 participants