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

config: introduce Import section #10421

Merged
merged 3 commits into from
May 14, 2024
Merged

config: introduce Import section #10421

merged 3 commits into from
May 14, 2024

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented May 8, 2024

This is an initial draft for an idea that @lidel and I were speaking about: a config section with the default data ingestion options. This would allow to easily change the defaults across the code-base easily in the future.

The largest problem at the moment is that the CID Version is, by default, 1 in some commands (dag put, block put) and 0 in others (add, except in cases where certain options are used). This was also the largest use case for this config section: being able to upgrade the default CID version in the future. However, we can't use this configuration option since not all commands use the same default at the moment.

My suggestion would be to investigate the possibility of changing the default of ipfs add to CIDv1, since this one seems to be the only one that still uses 0 as the default, except in some cases.

cc #4143

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

The goal here is to unblock upgrade to CIDv1 (#4143) by having a global configuration option for people that need to go back to the old defaults once we switch to CIDv1 everywhere.

CID selection in block put is deprecated. We don't want to change or touch anything in dag|block put, because the real world problem is just ipfs add. It is ok if DefaultCidVersion is only applied to ipfs add here.

config/import.go Show resolved Hide resolved
config/import.go Outdated Show resolved Hide resolved
@hacdias hacdias force-pushed the import-defaults branch 2 times, most recently from 021e436 to 95cbc78 Compare May 8, 2024 14:08
@hacdias hacdias requested a review from lidel May 8, 2024 14:14
@hacdias hacdias marked this pull request as ready for review May 8, 2024 14:14
@hacdias hacdias requested a review from a team as a code owner May 8, 2024 14:14
@lidel lidel mentioned this pull request May 13, 2024
9 tasks
@lidel lidel force-pushed the import-defaults branch 2 times, most recently from 564b7ac to 65052c7 Compare May 14, 2024 00:16
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

lgtm, pushed some additional tests, docs and two profiles for user convenience.

I think there is a small UX bug where we should error and not silently ignore cid version – details inline. Once addressed, and if my changes look good, feel free to merge.

core/commands/add.go Show resolved Hide resolved
@hacdias hacdias enabled auto-merge (squash) May 14, 2024 11:02
@hacdias hacdias force-pushed the import-defaults branch 2 times, most recently from 079dc04 to 099ce9c Compare May 14, 2024 14:09
@lidel
Copy link
Member

lidel commented May 14, 2024

Discussed this, and we decided to keep this simple and small.

This PR will not change the current behavior where passing --raw-leaves=true alone implies --cid-version 1 for the leaves.

We can do that when we make a "breaking change" and switch default to cidv1 (#4143), but for now we don't do anything that would impact existing users.

The purpose of this PR is to lay the ground for that change without forcing people to do it when they upgrade.

@hacdias hacdias merged commit 8022e13 into master May 14, 2024
14 checks passed
@hacdias hacdias deleted the import-defaults branch May 14, 2024 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: 🎉 Done
Development

Successfully merging this pull request may close these issues.

2 participants