Skip to content

Conversation

@stbrody
Copy link
Contributor

@stbrody stbrody commented Oct 23, 2020

No description provided.

}
],
link: "bafyreighkvhf5ghi5hyeoain4y5p5zn5k6dyoazkjso3gnwbsqi7tk32ze"
link: "bafyreiago3pnhq2r7yy4osv7h7yrja7vfkuyzvdiloqnhsk64qcior6tn4"
Copy link
Contributor Author

@stbrody stbrody Oct 23, 2020

Choose a reason for hiding this comment

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

This was the issue that was causing the test to fail. The test was finding this 'link' field was coming back with a value different than what it expected. I simply took what the test was actually finding and pasted it here into the expected slot. Not very good testing practices, as I didn't actually verify that this is the proper value that this field should really have. I'm not sure exactly how this field is calculated, but my guess is that it's derived from a hash of the data, and that buy changing "owners" to "controllers" I changed the resulting hash. Do you know how I would go about actually verifying that this is the proper expected value so that we know the test is actually testing the proper behavior and not just set to expect what it is outputting, regardless of whether that's correct?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes this is expected to change given that it's the CID (hash + additional data) of the genesis content above. I think it's safe to assume that this hash is correct, but you can try to change the genesis content again to make sure that that does indeed change this CID.
In general in dag-jose the link property is a CID instance of the base64url encoded payload. Not sure why they are different in this test. Probably because this object is created by some mock.

You can learn more about dag-jose here:
https://github.com/ceramicnetwork/js-dag-jose
ipld/specs#269

@stbrody stbrody requested a review from simonovic86 October 23, 2020 18:42
@stbrody stbrody marked this pull request as ready for review October 23, 2020 18:42
Copy link
Collaborator

@oed oed left a comment

Choose a reason for hiding this comment

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

Looks good. I'll let @simonovic86 decide when to merge this as it's a breaking change and it would likely make sense to release together with many of your other tickets @stbrody since they are also breaking changes.

}
],
link: "bafyreighkvhf5ghi5hyeoain4y5p5zn5k6dyoazkjso3gnwbsqi7tk32ze"
link: "bafyreiago3pnhq2r7yy4osv7h7yrja7vfkuyzvdiloqnhsk64qcior6tn4"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes this is expected to change given that it's the CID (hash + additional data) of the genesis content above. I think it's safe to assume that this hash is correct, but you can try to change the genesis content again to make sure that that does indeed change this CID.
In general in dag-jose the link property is a CID instance of the base64url encoded payload. Not sure why they are different in this test. Probably because this object is created by some mock.

You can learn more about dag-jose here:
https://github.com/ceramicnetwork/js-dag-jose
ipld/specs#269

@simonovic86
Copy link
Contributor

Looks good. I'll let @simonovic86 decide when to merge this as it's a breaking change and it would likely make sense to release together with many of your other tickets @stbrody since they are also breaking changes.

yep, I agree with @oed. It would make sense if we can put together all the breaking changes (renames, tweaks) under the one PR 👍 We can use this one.

@simonovic86 simonovic86 added chore various chores enhancement labels Oct 26, 2020
@simonovic86 simonovic86 added this to the Sprint 47 milestone Oct 26, 2020
Copy link
Contributor

@simonovic86 simonovic86 left a comment

Choose a reason for hiding this comment

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

LGTM! Let's put all breaking changes tweaks and renames under the same repo. It will be easier to handle merge conflicts, etc. Makes sense?

Just a note @stbrody the way of how we create branches now is:

  • feat/some-feature if it's a feature
  • fix/some-fix if it's a bug
  • chore/some-chore if it's a chore.

@stbrody stbrody merged commit c94ff15 into develop Oct 26, 2020
@stbrody stbrody linked an issue Oct 26, 2020 that may be closed by this pull request
@stbrody stbrody deleted the rename-owners branch October 26, 2020 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

chore various chores

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Rename "owners"

4 participants