Repoint kardianos/service to fork elastic/kardianos-service

[ycombinator]

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.

[elasticmachine]

Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane)

[mergify]

This pull request does not have a backport label. Could you fix it @ycombinator? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

• backport-v./d./d./d is the label to automatically backport to the 8./d branch. /d is the digit

NOTE: backport-skip has been added to this pull request.

[michel-laterman]

Do we have a follow up issue to change the import in our codebase?

[ycombinator]

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.

[leehinman]

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.

[ycombinator]

Done in 52c70f1.

[blakerouse]

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'?

[ycombinator]

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 kardianos in the name? Something else?

[blakerouse]

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.

[ycombinator]

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.

Ah, clever! I've renamed the repository to https://github.com/elastic/go-service and updated the reference in go.mod in this PR via 4c49871.

Did you also want me to get rid of the replace, directly require https://github.com/elastic/go-service, and change the imports in the code to match?

[blakerouse]

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.

Ah, clever! I've renamed the repository to https://github.com/elastic/go-service and updated the reference in go.mod in this PR via 4c49871.

Did you also want me to get rid of the replace, directly require https://github.com/elastic/go-service, and change the imports in the code to match?

Yes, please!

[ycombinator]

Did you also want me to get rid of the replace, directly require https://github.com/elastic/go-service, and change the imports in the code to match?

Yes, please!

This change depends on making this change first: elastic/go-service#6

[blakerouse]

@ycombinator I updated this to point to the new github.com/elastic/go-service and removed and cleaned up the items that where merged into go-service.

[ycombinator]

Integration tests are failing consistently on this PR. I'm going to temporarily remove the code changes in internal/pkg/agent/install/svc.go made as part of ba18e5e and see if that helps.

cc: @blakerouse

[elastic-sonarqube]

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarQube

[ycombinator]

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).