-
Notifications
You must be signed in to change notification settings - Fork 142
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
Repoint kardianos/service to fork elastic/kardianos-service #4859
Conversation
Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane) |
This pull request does not have a backport label. Could you fix it @ycombinator? 🙏
NOTE: |
go.mod
Outdated
@@ -291,6 +291,7 @@ replace ( | |||
github.com/Shopify/sarama => github.com/elastic/sarama v1.19.1-0.20220310193331-ebc2b0d8eef3 | |||
github.com/dop251/goja => github.com/andrewkroh/goja v0.0.0-20190128172624-dd2ac4456e20 | |||
github.com/dop251/goja_nodejs => github.com/dop251/goja_nodejs v0.0.0-20171011081505-adff31b136e6 | |||
github.com/kardianos/service => github.com/elastic/kardianos-service v0.0.0-20230215200102-9832e01049dd |
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.
Do we have a follow up issue to change the import in our codebase?
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 wasn't planning on changing the imports. Do you think we need to?
If we changed the imports, we could get rid of this replace directive and directly require github.com/elastic/kardianos-service
instead. Would we then also have to update the module name here: https://github.com/elastic/kardianos-service/blob/master/go.mod#L1?
Not saying I'm opposed to changing the imports, just wondering if we need to and, if so, what other changes might need to go hand in hand with that.
go.mod
Outdated
@@ -291,6 +291,7 @@ replace ( | |||
github.com/Shopify/sarama => github.com/elastic/sarama v1.19.1-0.20220310193331-ebc2b0d8eef3 | |||
github.com/dop251/goja => github.com/andrewkroh/goja v0.0.0-20190128172624-dd2ac4456e20 | |||
github.com/dop251/goja_nodejs => github.com/dop251/goja_nodejs v0.0.0-20171011081505-adff31b136e6 | |||
github.com/kardianos/service => github.com/elastic/kardianos-service v0.0.0-20230215200102-9832e01049dd |
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 go.mod will allow a comment, if so can you please add one saying why we need to the replace directive for kardianos. Sometimes it can be hard to trace back to original PR with the reason and long term it would be good for us to know why we have each each replace directive.
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.
Done in 52c70f1.
I would rather see this as a hard fork. Where we just maintain it as its own project. We have some other template changes in agent that could be merged as well, making it cleaner as well. What about naming it 'elastic/go-service'? |
I'm not opposed to this idea, considering it's probably unlikely that the upstream repo will become active again any time soon. But if the upstream repo did become active again some day, it would be nice to contribute our fixes back to the community. Just to understand the full picture, what are the downsides of keeping the fork as-is? Are the template changes you're mentioning not generic enough? Is it the |
The template changes are very generic, could also go upstream if they were active. The reason for request to use name 'elastic/go-service' is that it will only require updating the import and not the actual usage of the module in the code. Golang will remove the 'go-' in the front automatically allowing all the 'service.*' usage to remain the same in the code base. It's also very common naming scheme. I also just generally like it, haha. |
Ah, clever! I've renamed the repository to https://github.com/elastic/go-service and updated the reference in Did you also want me to get rid of the |
Yes, please! |
This change depends on making this change first: elastic/go-service#6 |
@ycombinator I updated this to point to the new |
2e68e8c
to
16b2e2a
Compare
f3425f7
to
cebefe5
Compare
cebefe5
to
560cd22
Compare
Integration tests are failing consistently on this PR. I'm going to temporarily remove the code changes in cc: @blakerouse |
This reverts commit ba18e5e.
Quality Gate passedIssues Measures |
* Repoint kardianos/service to fork elastic/kardianos-service * Adding comment * Rename elastic/kardianos-service to elastic/go-service * Change to github.com/elastic/go-service. * Running mage fmt --------- Co-authored-by: Blake Rouse <[email protected]> (cherry picked from commit 6e55d65) # Conflicts: # internal/pkg/agent/install/switch.go
I have zero recollection of merging this PR 😮. Even the local timestamp on the merge suggests I wouldn't have done it: CI is still failing so I'm not sure why / how this PR got merged. Reverting... [UPDATE] There was a change made to our CI pipeline configuration which, coupled with the fact that this PR here had auto-merge set, allowed this PR to get merged (even though tests were failing in CI). |
What does this PR do?
This PR repoints the dependency on github.com/kardianos/service to our fork of it at github.com/elastic/kardianos-service via a
replace
directive.Why is it important?
The upstream fork at https://github.com/kardianos/service does not seem to be actively maintained based on the latest commit timestamp and the lack of response to kardianos/service#396.
More pressingly, we need a fix to kardianos/service#395.
Disruptive User Impact
No.