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 support to replace directive in go.mod #3693

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

shravankshenoy
Copy link

Fixes #3492

About replace directive in go.mod

Replaces the content of a module at a specific version (or all versions) with another module version or with a local directory. Syntax as shown below

replace module-path [module-version] => replacement-path [replacement-version]

Detailed info at https://go.dev/doc/modules/gomod-ref

Changes made

  • Added regex to get module-path, module-version, replacement-path and replacement-version from replace directive
  • Wrote conditions to handle single line and multi line replace directives
  • Reran opencensus test to account for the replace directive
  • Added new test using milvus go.mod to test the replace directive functionality
    `

Signed-off-by: Shenoy <shravanshenoy1998@hotmail.com>
Copy link
Member

@AyanSinhaMahapatra AyanSinhaMahapatra left a comment

Choose a reason for hiding this comment

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

@shravankshenoy thanks for the PR!

I've added a couple of comments for your consideration, to the best of my knowledge (since I'm not too familiar with go build systems), please review and update the PR accordingly, as applicable.

@@ -188,6 +259,7 @@ def parse_gomod(location):

gomods.require = require
gomods.exclude = exclude
gomods.replace = replace
Copy link
Member

Choose a reason for hiding this comment

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

Also add replace as an attribute of class GoModule

Comment on lines +214 to +228
replace.append(
GoModule(
namespace=namespace,
name=name,
version=parsed_rep_link.group("version"),
)
)

replace.append(
GoModule(
namespace=replacement_namespace,
name=replacement_name,
version=parsed_rep_link.group("replacement_version"),
)
)
Copy link
Member

Choose a reason for hiding this comment

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

This is not correct, as we are treating the module to be replaced and the replacement module the same way. I think the first module in the replace directive (the module to be replaced) should be added as a dependency with scope as exclude and the second module in the replace directive (the replacement module) should be added with scope as require.

@pombredanne what do you think?

@@ -0,0 +1,255 @@
module github.com/milvus-io/milvus
Copy link
Member

Choose a reason for hiding this comment

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

this is quite a large test file, would be nice to truncate this to only have the essential portion required to test the functionality, or find a shorter example. Also add the go.mod files linked in the main issue as tests.

github.com/bketelsen/crypt => github.com/bketelsen/crypt v0.0.4 // Fix security alert for core-os/etcd
github.com/expr-lang/expr => github.com/SimFG/expr v0.0.0-20231218130003-94d085776dc5
github.com/go-kit/kit => github.com/go-kit/kit v0.1.0
github.com/milvus-io/milvus/pkg => ./pkg
Copy link
Member

Choose a reason for hiding this comment

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

In the case where the replace directive contains a reference to a local directory, we can try to resolve this in the assemble step of the datafile handler, if there are package manifests in the local directory with package data for the replacement module.

  1. On the parse function side we store the local references in the extra_data attribute somehow, with the referenced path.
  2. On the assembly side, we try to walk the codebase for this referenced directory.
  3. If the referenced directory is present in the codebase, and we have parsed go package data present there, we use that to populate the required dependency list.

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.

Support directives in go.mod
2 participants