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

fix: repo rename #740

Merged
merged 5 commits into from
Jan 16, 2025
Merged

fix: repo rename #740

merged 5 commits into from
Jan 16, 2025

Conversation

ShubhamChaturvedi7
Copy link
Contributor

Issue #, if available:

Description of changes:

Squash/merge commit message, if applicable:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

github.com/aws/aws-cryptographic-material-providers-library/releases/go/kms v0.0.0
github.com/aws/aws-cryptographic-material-providers-library/releases/go/mpl v0.0.0
github.com/aws/aws-cryptographic-material-providers-library/releases/go/primitives v0.0.0
github.com/aws/aws-cryptographic-material-providers-library/releases/go/dynamodb v0.0.3
Copy link
Member

Choose a reason for hiding this comment

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

Isn't our process replacing this package with the local version in runtime/go. But put the actual version with no replacement in releases/go directory?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do have a replace in place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also made changes to do a version agnostic replace.

Copy link
Member

Choose a reason for hiding this comment

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

Non blocking but I would rather put v0.0.0 (a version thats not released) in require to signal replace directives is used in the go mod.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really - I'd rather have the version number and do a replace. The issue with v0.0.0 is if you really want to test against a version you have to explicitly specify it and then go mod and stuff. With the right version in place, you can just comment out the replace directive and it will work.

Copy link
Member

@rishav-karanjit rishav-karanjit left a comment

Choose a reason for hiding this comment

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

LGTM

github.com/aws/aws-cryptographic-material-providers-library/releases/go/kms v0.0.0
github.com/aws/aws-cryptographic-material-providers-library/releases/go/mpl v0.0.0
github.com/aws/aws-cryptographic-material-providers-library/releases/go/primitives v0.0.0
github.com/aws/aws-cryptographic-material-providers-library/releases/go/dynamodb v0.0.3
Copy link
Member

Choose a reason for hiding this comment

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

Non blocking but I would rather put v0.0.0 (a version thats not released) in require to signal replace directives is used in the go mod.

@rishav-karanjit
Copy link
Member

Non blocking: this might be more of a refactor commit then a fix commit.

@ShubhamChaturvedi7
Copy link
Contributor Author

Non blocking: this might be more of a refactor commit then a fix commit.

Not really - I'd rather have the version number and do a replace. The issue with v0.0.0 is if you really want to test against a version you have to explicitly specify it and then go mod and stuff. With the right version in place, you can just comment out the replace directive and it will work.

@ShubhamChaturvedi7 ShubhamChaturvedi7 merged commit ae8b4b7 into mainline Jan 16, 2025
84 checks passed
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.

2 participants