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

Changes needed for --cid-base option in go-ipfs (simplified vesion) #10

Merged
merged 4 commits into from
Jan 7, 2019

Conversation

kevina
Copy link
Contributor

@kevina kevina commented Dec 12, 2018

Simplified version of #8

Do not merge until ipfs/kubo#5789 is approved.

@kevina
Copy link
Contributor Author

kevina commented Dec 12, 2018

@Stebalien when gx publishing this should it I use minor or patch I would think minor since it adds a new package.

@kevina kevina changed the title WIP: Create a CID encoder that optionally upgrads a CIDv0 to CIDv1 WIP: Changes needed for --cid-base option in go-ipfs (simplified vesion) Dec 12, 2018
Copy link
Member

@magik6k magik6k left a comment

Choose a reason for hiding this comment

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

Code LGTM, but it would be really nice to get some tests, especially for --filter in cid-fmt

Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

Same as @magik6k. LGTM but needs tests.

@kevina kevina changed the title WIP: Changes needed for --cid-base option in go-ipfs (simplified vesion) Changes needed for --cid-base option in go-ipfs (simplified vesion) Jan 7, 2019
@kevina
Copy link
Contributor Author

kevina commented Jan 7, 2019

Okay I added tests and this should be good to go. Will merge once I get an approving review.

@magik6k
Copy link
Member

magik6k commented Jan 7, 2019

Can you enable CI here? Should be as simple as mkdir ci; echo 'golang()' > ci/Jenkinsfile + giving ci team on github write access (to set commit status)

cidenc/encoder.go Outdated Show resolved Hide resolved
cidenc/encoder.go Outdated Show resolved Hide resolved
@kevina
Copy link
Contributor Author

kevina commented Jan 7, 2019

Someone with the write permissions will need to activate Jenkins and Travis...

@Stebalien Stebalien merged commit d5fe12a into master Jan 7, 2019
@ghost ghost removed the status/in-progress In progress label Jan 7, 2019
@Stebalien
Copy link
Member

I've activated travis. Not sure how to activate Jenkins.

@Stebalien
Copy link
Member

(I assume this was OK to merge... If not, we can revert it)

@kevina
Copy link
Contributor Author

kevina commented Jan 8, 2019

I assume this was OK to merge... If not, we can revert it

No, not really, the commits to activate the CI's where untested and I was waiting for them to be activated before I merged this PR. If that did not happen I would of removed the commit from the P.R. to do it separately. In fact the Travis CI need some fixing.

Reverting will just make this messier I will create another P.R.

@magik6k
Copy link
Member

magik6k commented Jan 8, 2019

Not sure how to activate Jenkins

Just give ci team on GitHub write access to this repo

@Stebalien
Copy link
Member

@magik6k done.

@kevina got it. Merged your fix as everything seems to work now.

@kevina
Copy link
Contributor Author

kevina commented Jan 8, 2019

@Stebalien This needs to be gx-published. Should we bump the minor version or just the patch level? I would think minor since it adds a new package.

@kevina
Copy link
Contributor Author

kevina commented Jan 8, 2019

@Stebalien This needs to be gx-published. Should we bump the minor version or just the patch level? I would think minor since it adds a new package.

In the interest of pushing ipfs/kubo#5789 forward, I want ahead and gx published this as a minor release.

@Stebalien
Copy link
Member

Either way, just don't bump to version 1.0.

For packages <= 1.0, I usually only bump the minor for breaking changes but that's mostly a habit from working with rust/js packages (https://nodesource.com/blog/semver-tilde-and-caret/#caretmajorzero).

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.

3 participants