Skip to content

Migrations#327

Merged
aschmahmann merged 21 commits into
masterfrom
migrations
Mar 31, 2021
Merged

Migrations#327
aschmahmann merged 21 commits into
masterfrom
migrations

Conversation

@gammazero

@gammazero gammazero commented Jan 12, 2021

Copy link
Copy Markdown
Contributor

This PR creates individual distributions, for each IPFS fs repo migration. This enables migration tools to download a minimal set of migrations needed to go from one version of IPFS to another.

The individual migrations will be available from the ipfs network, but are not shown on the IPFS distributions web page**. Each migration has the same ipfs path structure as any other migration, so it can easily be found even in the absence of a link on the distributions web page. For example, the ipfs-10-to-11 migration can be found at https://dist.ipfs.io/ipfs-10-to-11 and will have the same contents as other migrations. NOTE: The migrations will not be published to IPFS until after approval/merge of this PR.

This PR requires, and is built from, #320

Addresses issue #326

Additional modifications to the build scripts:

  • Allow an optional sub-package to be specified when creating a new distribution
  • Do not build web content if distribution contains no-site file
  • Fix minor cleanup issues
  • Fix CI issue with jq not being able to read files directly

** NOTE: There is a no-site file in each migration distribution that prevents building the web page content for that distribution.

CID: QmVxxcTSuryJYdQJGcS8SyhzN7NBNLTqVPAxpu6gp2ZcrR

@welcome

welcome Bot commented Jan 12, 2021

Copy link
Copy Markdown

Thank you for submitting this PR!
A maintainer will be here shortly to review it.
We are super grateful, but we are also overloaded! Help us by making sure that:

  • The context for this PR is clear, with relevant discussion, decisions
    and stakeholders linked/mentioned.

  • Your contribution itself is clear (code comments, self-review for the
    rest) and in its best form. Follow the code contribution
    guidelines

    if they apply.

Getting other community members to do a review would be great help too on complex PRs (you can ask in the chats/forums). If you are unsure about something, just leave us a comment.
Next steps:

  • A maintainer will triage and assign priority to this PR, commenting on
    any missing things and potentially assigning a reviewer for high
    priority items.

  • The PR gets reviews, discussed and approvals as needed.

  • The PR is merged by maintainers when it has been approved and comments addressed.

We currently aim to provide initial feedback/triaging within two business days. Please keep an eye on any labelling actions, as these will indicate priorities and status of your contribution.
We are very grateful for your contribution!

@jessicaschilling

Copy link
Copy Markdown
Contributor

@Stebalien Are you able to review this, or pass along to someone else who can? Thank you!

@Stebalien Stebalien left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks reasonable to me. I didn't check every change in detail, but if it works, it works. And if something goes wrong, a manual "diff" should catch that.

Comment thread build-go.sh Outdated
Comment thread build-go.sh
Comment thread scripts/patch.js
@jessicaschilling

Copy link
Copy Markdown
Contributor

@olizilla Are you wanting to wait to merge this until #320?

@olizilla

Copy link
Copy Markdown
Member

This PR builds on top of #320, so we should merge #320 first. More generally for merge decisions on this repo, I defer to Adin as he has to use this to release go-ipfs and Steven as the most prolific contributor.

@aschmahmann aschmahmann left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. @gammazero it looks like you've also fixed and cleaned up a few unrelated things.

I left some questions/comments trying to clarify this to make sure A) I understand B) that if we check into why a change was made a few months from now we know why.

Comment thread Makefile
Comment thread build-go.sh
Comment thread build-go.sh
Comment thread dist.sh
Comment on lines -40 to +54
sed "s/ABCGHREPOXYZ/$(sedEscapeArg "$repo")/g" templates/Makefile | sed "s/ABCDISTNAMEXYZ/$name/g" > "dists/$name/Makefile"
sed "s/github.com\/foo\/bar/$(sedEscapeArg "$repo")/g" templates/Makefile | sed "s/cmd\/bar/$subpkg/g" > "dists/$name/Makefile"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this just an ancient bug fix?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes. I was a bug leftover from when the string ABCDISTNAMEXYZ appeared in the Makefile template.

Comment thread dist.sh
Comment thread dist.sh
Comment thread templates/build_matrix
Comment thread scripts/patch.js Outdated
const event = new Date()
const DIST_DOMAIN = 'dist.ipfs.io'
const MFS_DIR = `/${DIST_DOMAIN}`
const MFS_DIR = `/${DIST_DOMAIN}` + '_' + event.toISOString()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This means that people who run this will end up with one MFS reference per-build instead of one per dist website. Given that my understanding is we're pinning all the files we add anyway the only problem that could come up with this is just it polluting your MFS root space.

nit: WDYT about having some nesting to keep ipfs files ls / cleaner? An example might be /${DIST_DOMAIN}/${DIST_DOMAIN} + '_' + event.toISOString()`

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I actually want to pollute MFS root. The is so I see that I have multiple dists and remember to remove those that I no longer need. This is not for the purpose of archiving dists, but to avoid stepping on a previous one. Once I am happy with a dist, I expect to remove previous ones.

@Stebalien suggested in the previous review, that I use a random string here. I decided that date was more useful to determine which was the most recent.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Honestly, keeping old ones should be fine as it shouldn't actually use any more data. It also makes it easier to delete them (delete the root).

But either way. I don't have strong opinions.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this is just a local thing for the builder we can always change later if it becomes a problem with no significant consequences, so we can just leave it as is and figure it out.

@gammazero gammazero force-pushed the migrations branch 2 times, most recently from 31ab6cd to 492ae26 Compare February 23, 2021 23:11
- Optional argument to dist.sh to specify sub-package
- Fix Makefile to allow dist.sh to correctly edit variables
- Do not generate web site data if distribution contains "no-site" file.
Each fs repo migration is build as a separate distribution
- Remove filtered_versions files
- Do not create filtered_versions files
- Do not check that version files are sorted since that does not work with semver sorting.
Update migrations version to match current fs-repo-migrations.
@aschmahmann aschmahmann merged commit eef9d68 into master Mar 31, 2021
@gammazero gammazero deleted the migrations branch April 2, 2021 05:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants