Skip to content

Decouple CreateCryptoConfig() from github.com/urfave/cli#56

Merged
stefanberger merged 1 commit intocontainerd:masterfrom
AkihiroSuda:split-create-crypto-config
Oct 29, 2021
Merged

Decouple CreateCryptoConfig() from github.com/urfave/cli#56
stefanberger merged 1 commit intocontainerd:masterfrom
AkihiroSuda:split-create-crypto-config

Conversation

@AkihiroSuda
Copy link
Member

Decouple CreateCryptoConfig() from github.com/urfave/cli, so that it can be called from other applications that do not use github.com/urfave/cli.

@AkihiroSuda AkihiroSuda force-pushed the split-create-crypto-config branch 2 times, most recently from 7cee4ee to a6cfadb Compare October 27, 2021 10:12
@AkihiroSuda
Copy link
Member Author

@stefanberger @lumjjb PTAL, can we have a new release with this?

@stefanberger
Copy link
Contributor

LGTM.

lumjjb
lumjjb previously approved these changes Oct 27, 2021
Copy link
Collaborator

@lumjjb lumjjb left a comment

Choose a reason for hiding this comment

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

EDIT: @AkihiroSuda I think we moved this function as part of ocicrypt, thoughts on calling it directly from ocicrypt library

https://github.com/containers/ocicrypt/blob/main/helpers/parse_helpers.go#L298

@lumjjb lumjjb dismissed their stale review October 27, 2021 13:18

additional comment

@stefanberger
Copy link
Contributor

EDIT: @AkihiroSuda I think we moved this function as part of ocicrypt, thoughts on calling it directly from ocicrypt library

https://github.com/containers/ocicrypt/blob/main/helpers/parse_helpers.go#L298

Iirc parse_helpers.go in the two projects were a bit different with imgcrypt's version supporting pgp/gpg better.

@AkihiroSuda
Copy link
Member Author

EDIT: @AkihiroSuda I think we moved this function as part of ocicrypt, thoughts on calling it directly from ocicrypt library
https://github.com/containers/ocicrypt/blob/main/helpers/parse_helpers.go#L298

Iirc parse_helpers.go in the two projects were a bit different with imgcrypt's version supporting pgp/gpg better.

The different part seems commented out https://github.com/containers/ocicrypt/blob/e4a936881fb7cf4b2b8fe49e81b8232fd4c48e97/helpers/parse_helpers.go#L234-L260

What was the reason to comment out this?
Can we unify the two implementations?

@stefanberger
Copy link
Contributor

I would let the unification of the parser_helpers happen independent of this PR... eventually it should happen.

@lumjjb
Copy link
Collaborator

lumjjb commented Oct 28, 2021

It i recall correctly, we commented it out in the ocicrypt library since it added some dependencies on gpg binary, which was not favorable for cri-o. I am in agreement with taking this PR first and resolving it separately as @stefanberger recommended.

"github.com/urfave/cli"
)

type Spec struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
type Spec struct {
type EncArgs struct {

minor nit, suggestion so that there is no confusion with the oci spec.

Copy link
Member Author

Choose a reason for hiding this comment

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

updated

@lumjjb
Copy link
Collaborator

lumjjb commented Oct 28, 2021

minor suggestion, lmk what you think, else we can merge it and create a new release

@AkihiroSuda AkihiroSuda force-pushed the split-create-crypto-config branch from a6cfadb to 1b763e7 Compare October 29, 2021 05:47
Decouple `CreateCryptoConfig()` from `github.com/urfave/cli`, so that it
can be called from other applications that do not use `github.com/urfave/cli`.

Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
@AkihiroSuda AkihiroSuda force-pushed the split-create-crypto-config branch 2 times, most recently from b00e927 to fe5e256 Compare October 29, 2021 06:32
@stefanberger stefanberger merged commit 902158d into containerd:master Oct 29, 2021
@stefanberger
Copy link
Contributor

Will tag as v1.1.2 then, ok?

@stefanberger
Copy link
Contributor

Pushed tag now.

@lumjjb
Copy link
Collaborator

lumjjb commented Oct 29, 2021

we should also be part of that tag to update it to ocicrypt 1.1.2 as well
https://github.com/containerd/imgcrypt/blob/master/go.mod#L12

@stefanberger
Copy link
Contributor

stefanberger commented Oct 29, 2021

I deleted the v1.1.2 tag and by accident also the v1.1.1 tag. Can yo send a PR with the necessary ocicrypt updates?

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