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

Add x509 parsing utilities #1

Merged
merged 3 commits into from
May 19, 2022
Merged

Add x509 parsing utilities #1

merged 3 commits into from
May 19, 2022

Conversation

rgnote
Copy link
Contributor

@rgnote rgnote commented May 13, 2022

  • Moving x509 cert, key parsing utilities from notation-go to notation-core-go
  • Will raise a separate PR for refactoring notation-go and notation repos

Signed-off-by: rgnote [email protected]

go.mod Outdated
@@ -0,0 +1,3 @@
module github.com/notaryproject/notation-core-go

go 1.18
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want someone with more familiarity with golang chime in here. Currently, notation and notation-go are on 1.17. Reading 1.18 release notes, it looks like 1.18 is backward compatible and it is also fixing some bugs present in earlier Go versions. Let me know if there a reason we shouldn't upgrade to 1.18.

Copy link

Choose a reason for hiding this comment

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

@shizhMSFT , @qmuntal, any feedback here?

Copy link

Choose a reason for hiding this comment

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

The go directive inside go.mod does not refer to the go version that will be used to compile the library, but to the maximum set of language features that this library supports. For example, generics are only available if we set it to 1.18 and embed if we set it to 1.16 or greater.

A customer using a lower Go toolchain version can still use a module whose go directive if set to a higher version unless the module really uses a newer language feature.

The best practice is to set the go directive to the lowest supported Go version. As we are not using generics here, I would set it to 1.17 to be in-line with notation-go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good to know. Updated the PR.

@gokarnm Can you merge if this looks good?

x509/cert.go Outdated Show resolved Hide resolved
@dtzar
Copy link

dtzar commented May 19, 2022

LGTM

rgnote and others added 3 commits May 19, 2022 13:12
Signed-off-by: rgnote <[email protected]>
Co-authored-by: Milind Gokarn <[email protected]>
Signed-off-by: rgnote <[email protected]>
Copy link

@NiazFK NiazFK left a comment

Choose a reason for hiding this comment

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

LGTM

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.

6 participants