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 helia-script-tag example #13

Merged
merged 16 commits into from
Mar 31, 2023

Conversation

SgtPooki
Copy link
Contributor

@SgtPooki SgtPooki commented Mar 16, 2023

  • feat(script-tag): initial commit
  • feat(helia-script-tag): fluff up example

Note: I do not have permissions to create ipfs-examples/helia-script-tag repo.

demo: https://codesandbox.io/p/sandbox/helia-script-tag-g420c3

codepen example also available: https://codepen.io/SgtPooki/pen/dyqKVqx

Copy link
Contributor

@whizzzkid whizzzkid left a comment

Choose a reason for hiding this comment

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

nice! I can see the node go online! but from a reader's point of view I think we can change a few things.

Copy link
Contributor

Choose a reason for hiding this comment

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

I saw this in other examples too, should this file be in the root like .github/workflows/ci.yml

Copy link
Contributor

Choose a reason for hiding this comment

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

No, this gets copied to the per-example repo and is used when a user tries to open a PR there - for example: https://github.com/ipfs-examples/helia-101/blob/main/.github/pull_request_template.md

If we placed this in the root of this repo, it would show a "Please don't open a PR here" message when someone tried to open a PR here, which is not what we want.

Copy link

Choose a reason for hiding this comment

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

Is there some documentation we should add so that is clearer for contributors? What would have helped you @whizzzkid ?

Copy link
Contributor

Choose a reason for hiding this comment

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

We can have an example generator which updates and maintains boilerplate stuff.

This looks quite hectic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the generator is simply forking an example and then continuing. I don't want to maintain a generator, and I don't want to break the ipfs-examples or fork&go pattern without good reason and less work

Copy link
Contributor

Choose a reason for hiding this comment

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

I see this in other examples too, is this only to sync feature branches?

Questions (Most likely for @achingbrain):

  • can't this be in the root alongside workflow.yml
  • github has a setting to force branch update and merge queue which also achieves something similar, how's this different? seems boilerplate overhead.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is copied to the per-example repo and is then triggered by CI on this repo, it pulls a subfolder from this repo to the per-example repo and replaces all the files there.

I would rather the changes were pushed from a build on this repo instead of triggering a pull from every per-example repo but GitHub's permissions model prevented it, at the time at least.

If there's a better way of doing this PRs are gratefully accepted!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we could do similar to what exists in our {org}/.github-mgmt repos, or unified CI, where files that are to be copied to other repos are in a central location, and we copy/override example subfolders & commit before pushing to example repos.

This is probably already supported by https://github.com/ipfs-examples/github-mgmt with the repositories.*.files.content key for each repo

examples/helia-script-tag/README.md Outdated Show resolved Hide resolved
examples/helia-script-tag/README.md Outdated Show resolved Hide resolved
examples/helia-script-tag/README.md Show resolved Hide resolved
examples/helia-script-tag/src/index.js Outdated Show resolved Hide resolved
examples/helia-script-tag/src/index.js Show resolved Hide resolved
examples/helia-script-tag/src/index.js Outdated Show resolved Hide resolved
examples/helia-script-tag/src/index.js Show resolved Hide resolved
examples/helia-script-tag/src/index.js Show resolved Hide resolved
Copy link

@BigLep BigLep left a comment

Choose a reason for hiding this comment

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

@SgtPooki :

  1. you can request permissions at https://github.com/ipfs-examples/github-mgmt
  2. I don't think we need to provide signposts back to js-ipfs-examples. This can live on its own.

Copy link

Choose a reason for hiding this comment

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

Is there some documentation we should add so that is clearer for contributors? What would have helped you @whizzzkid ?

@SgtPooki
Copy link
Contributor Author

  1. you can request permissions at https://github.com/ipfs-examples/github-mgmt

ipfs-examples/github-mgmt#17

  1. I don't think we need to provide signposts back to js-ipfs-examples. This can live on its own.

I was thinking similar, but I wanted to fill out the readme appropriately. i'll look to the original example's readme and see what I can pull here instead of calling it out.

examples/helia-script-tag/README.md Outdated Show resolved Hide resolved
examples/helia-script-tag/README.md Show resolved Hide resolved
examples/helia-script-tag/src/createLibp2pInstance.js Outdated Show resolved Hide resolved
@SgtPooki SgtPooki requested a review from BigLep March 28, 2023 21:45
@BigLep
Copy link

BigLep commented Mar 29, 2023

Given we're factoring out the common stuff after, it seems good to me on that front.
It would be ideal to get a code approval from someone else.

@SgtPooki SgtPooki merged commit cf3e81c into ipfs-examples:main Mar 31, 2023
@SgtPooki SgtPooki deleted the example/helia-script-tag branch March 31, 2023 18:11
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.

4 participants