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(connector-iroha2): update to the new LTS image as of 28.07.2023 #2252

Merged
merged 1 commit into from
Aug 8, 2023

Conversation

outSH
Copy link
Contributor

@outSH outSH commented Jan 9, 2023

  • Change iroha2 setup docker and helper classes to work with the new LTS image.
  • Update Iroha SDK packages to the newest.
  • Fix some tests that were failing after upgrade.
  • Adjust SDK usage (new version doesn't create Torii client,
    arguments are provided with each method instead)
  • Use pinned iroha2 container version in all in one image.
  • Use skopeo to pre-download pinend image version.

@petermetz
Copy link
Contributor

@outSH Just bumping the thread to make sure we don't forget: Has this reached a point in the meantime where it could be merged? I'm trying to clear out some of the backlog of the pull requests that we have.

@outSH
Copy link
Contributor Author

outSH commented Jul 26, 2023

@outSH Just bumping the thread to make sure we don't forget: Has this reached a point in the meantime where it could be merged? I'm trying to clear out some of the backlog of the pull requests that we have.

I will update it shortly

@outSH outSH force-pushed the iroha2_update_to_new_lts branch 2 times, most recently from d1383c3 to a7ed0c7 Compare July 27, 2023 14:11
@petermetz
Copy link
Contributor

I will update it shortly

@outSH Cheers! One small nit while you do that update: Please specify the exact version of said LTS so that the next time we do the exact same thing (update to the LTS image) the commit messages are still unique.

@petermetz petermetz added the Iroha Bugs/features related to the Iroha ledger connector plugin label Jul 28, 2023
@outSH outSH changed the title feat(connector-iroha2): update to the new LTS image feat(connector-iroha2): update to the new LTS image as of 28.07.2023 Jul 28, 2023
@outSH
Copy link
Contributor Author

outSH commented Jul 28, 2023

Cheers! One small nit while you do that update: Please specify the exact version of said LTS so that the next time we do the exact same thing (update to the LTS image) the commit messages are still unique.

@petermetz Well, they don't really publish any LTS versions, but I see your point and agree, and hence added the as of 28.07.2023 to PR and commit message to clarify it in the future.

I've reached Iroha team to provide pinned LTS docker images but didn't receive any answer. I wanted to use the pinned version in tests to prevent issues where they introduce some change or a bug to LTS version that breaks our tests, and we'll end up investigating the root cause wondering if it's connector or upstream iroha fault (I hope the motivations are clear).

Funny thing is, we didn’t have to wait for them at all - I've recently discovered that you can pull images using image digest, completely ignoring the tag. Seems kinda obvious that such feature exist, but I was not aware of that :(

I've switched to using a digest instead of a tag, but then the freeze-image script stopped working (it doesn't support this format), and since it is internal tool of another project that's hard to update, I've decided to use Red Hat's skopeo for pre-downloading the images - everything seem to work fine now.

Please review and upload iroha2-all-in-one image from this branch (tools/docker/iroha2-all-in-one/README.md) to hyperledger ghcr, so I can replace references to private repo.

@outSH outSH marked this pull request as ready for review July 28, 2023 12:00
@petermetz petermetz self-assigned this Jul 29, 2023
@petermetz
Copy link
Contributor

@petermetz Well, they don't really publish any LTS versions, but I see your point and agree, and hence added the as of 28.07.2023 to PR and commit message to clarify it in the future.

@outSH Thank you very much!

I've reached Iroha team to provide pinned LTS docker images but didn't receive any answer. I wanted to use the pinned version in tests to prevent issues where they introduce some change or a bug to LTS version that breaks our tests, and we'll end up investigating the root cause wondering if it's connector or upstream iroha fault (I hope the motivations are clear).

If you have a link to the github issue you opened or anything else that I can go and upvote please send the link and I'll do it to express more support for the idea and hopefully that way they'll prioritize it a little higher. This is important to protect against supply chain attacks IMO.

Funny thing is, we didn’t have to wait for them at all - I've recently discovered that you can pull images using image digest, completely ignoring the tag. Seems kinda obvious that such feature exist, but I was not aware of that :(

I've switched to using a digest instead of a tag, but then the freeze-image script stopped working (it doesn't support this format), and since it is internal tool of another project that's hard to update, I've decided to use Red Hat's skopeo for pre-downloading the images - everything seem to work fine now.

Yay! Thank you so much for investigating and going the extra mile so that we have a way of pinning it, definitely saves us from bugs and/or vulnerabilities down the line!

Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

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

@outSH I published it as ghcr.io/hyperledger/cactus-iroha2-all-in-one:2023-07-29-f2bc772ee please update and then pass it back for review!

@outSH
Copy link
Contributor Author

outSH commented Jul 31, 2023

I published it as ghcr.io/hyperledger/cactus-iroha2-all-in-one:2023-07-29-f2bc772ee please update and then pass it back for review!

Thank you, done!

If you have a link to the github issue you opened or anything else that I can go and upvote please send the link and I'll do it to express more support for the idea and hopefully that way they'll prioritize it a little higher. This is important to protect against supply chain attacks IMO.

Well, it ways mostly "internal" channel communication but since the digest pull seem to work fine I don't think there's a reason to push it forward anymore

@outSH outSH requested a review from petermetz July 31, 2023 12:23
Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

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

@outSH LGTM, thank you!

@petermetz
Copy link
Contributor

petermetz commented Jul 31, 2023

Well, it ways mostly "internal" channel communication but since the digest pull seem to work fine I don't think there's a reason to push it forward anymore

@outSH OK, that's fine by me. I'll go ahead and open an issue for this anyway on their repo because while the digests technically work, it is more mental effort for humans to deal with them and that increases the probability of somebody in the future slipping up because they didn't notice that the digest was different or even if they did they didn't realize that they were upgrading to something bad (which they would've if they were able to refer to their images via semver tags)

hyperledger-iroha/iroha#3771

Copy link
Contributor

@jagpreetsinghsasan jagpreetsinghsasan left a comment

Choose a reason for hiding this comment

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

LGTM

@jagpreetsinghsasan jagpreetsinghsasan dismissed their stale review August 3, 2023 06:53

Peter has already approved this PR. Removing my approval as we need approvals from maintainers from other organizations

Copy link
Contributor

@izuru0 izuru0 left a comment

Choose a reason for hiding this comment

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

LGTM

- Change iroha2 setup docker and helper classes to work with the new LTS image.
- Update Iroha SDK packages to the newest.
- Fix some tests that were failing after upgrade.
- Adjust SDK usage (new version doesn't create Torii client,
  arguments are provided with each method instead)
- Use pinned iroha2 container version in all in one image.
- Use skopeo to pre-download pinend image version.

Signed-off-by: Michal Bajer <[email protected]>
@petermetz petermetz enabled auto-merge (rebase) August 8, 2023 23:18
@petermetz petermetz merged commit ccdaa12 into hyperledger-cacti:main Aug 8, 2023
104 of 117 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Iroha Bugs/features related to the Iroha ledger connector plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants