-
Notifications
You must be signed in to change notification settings - Fork 8
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: watch for changes in replace paths #1323
Conversation
@@ -42,5 +42,3 @@ require ( | |||
golang.org/x/text v0.14.0 // indirect | |||
google.golang.org/protobuf v1.33.0 // indirect | |||
) | |||
|
|||
replace github.com/TBD54566975/ftl => /Users/dli/Development/ftl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@deniseli hopefully this doesn't break anything you were testing before, but assuming we don't want your local path here. Lemme know if you want me to change this back or to something else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be turned into a relative replace path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Word! Will update.
buildengine/filehash.go
Outdated
fullPath := filepath.Join(baseDir, pattern) | ||
|
||
// Remove any file-globbing patterns which could include '*' or '**' | ||
var dirPath string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's a function in the glob package we use to return the base directory of the glob.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OMG that would be so nice, this felt so overkill when I was writing it
@@ -42,5 +42,3 @@ require ( | |||
golang.org/x/text v0.14.0 // indirect | |||
google.golang.org/protobuf v1.33.0 // indirect | |||
) | |||
|
|||
replace github.com/TBD54566975/ftl => /Users/dli/Development/ftl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be turned into a relative replace path.
common/moduleconfig/moduleconfig.go
Outdated
return nil, fmt.Errorf("failed to parse %s: %w", goModPath, err) | ||
} | ||
|
||
replacements := reflect.DeepCopy(goModFile.Replace) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be cleaner to convert this into a map[string]string
and pass that around rather than cloning and mutating this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, yeah. Will update. I saw this used in another part of the code like this so assumed there was a reason, but a map would simplify this. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the other location we were updating a go.mod file with changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahhh 👍
8d9359e
to
29f3c0f
Compare
Is this going in? |
I was waiting for the other repo to be upgradable so I could test this change a bit before merging. Testing and merging today. |
29f3c0f
to
08644bf
Compare
😍 |
Fixes #1271