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

feat: add fs-repo-12-to-13 #162

Merged
merged 10 commits into from
Dec 12, 2022
Merged

feat: add fs-repo-12-to-13 #162

merged 10 commits into from
Dec 12, 2022

Conversation

Jorropo
Copy link
Contributor

@Jorropo Jorropo commented Dec 8, 2022

This needs

  • webtransport
    • unit tests in config_test.go
  • merge config cleanup from feat(12-13): removing hardcoded defaults #161
  • end-to-end tests to ensure (1) migration works and (2) is reversible
  • CI is green
    • disable CI from running things older than 11-to-12 - a lot of this code does not work with latest go anyway, we have them in this repo only for historical purpose.
  • merge and release to dist.ipfs.tech

This needs tests.
@lidel lidel changed the title feat: 12 to 13 feat: add fs-repo-12-to-13 Dec 9, 2022
@lidel
Copy link
Member

lidel commented Dec 9, 2022

Merged config cleanup migrations into this PR, and included unit test for them + updated this issue with remaining tasks.

This removes the need for running all legacy migrations every time
`make test` or `make build` is executed.

"Archived" migrations are added to ignored-migrations and that
will ignore them during the build.

I also removed iptb because it is used only by ignored migrations,
and not having to resolve its dependency issues saves us days
of useless work.
Copy link
Member

@ajnavarro ajnavarro left a comment

Choose a reason for hiding this comment

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

Can we have a list of things that we are migrating? that will make it easier for the code review to know what to check as the output.

@@ -0,0 +1,59 @@
// Package atomicfile provides the ability to write a file with an eventual
Copy link
Member

Choose a reason for hiding this comment

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

Should this file be on tools package?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably, this doesn't need to be done now given that would be function-less code change.
It would help making future migrations cleaner.

Copy link
Member

@ajnavarro ajnavarro left a comment

Choose a reason for hiding this comment

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

Added some comments.

Could you remove vendor folder? I'll do a second review after that.

fs-repo-12-to-13/migration/config.go Show resolved Hide resolved
fs-repo-12-to-13/migration/config.go Outdated Show resolved Hide resolved
fs-repo-12-to-13/migration/config.go Outdated Show resolved Hide resolved
fs-repo-12-to-13/migration/config_test.go Outdated Show resolved Hide resolved
fs-repo-12-to-13/migration/migration.go Show resolved Hide resolved
Keeping user config intact when any of relevant fields was customized
Copy link
Contributor Author

@Jorropo Jorropo left a comment

Choose a reason for hiding this comment

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

Comments have been addresses, it looks good to me, the CI is green we have tests, so I'm merging now, I'll make a fix if we find something broken with the RC.

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
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants