Conversation
|
This pull request does not have a backport label. Could you fix it @swiatekm? 🙏
|
7e258a6 to
8665824
Compare
4cace78 to
3ce8414
Compare
|
This pull request is now in conflicts. Could you fix it? 🙏 |
9f2ba9e to
7652b26
Compare
9832e34 to
efdadd0
Compare
|
This pull request is now in conflicts. Could you fix it? 🙏 |
de42544 to
fba2c76
Compare
|
This pull request is now in conflicts. Could you fix it? 🙏 |
3cac0f5 to
1c662de
Compare
|
Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane) |
# Conflicts: # dev-tools/mage/settings.go # Conflicts: # dev-tools/mage/packageversion.go
4449a1a to
ac8d911
Compare
💚 Build Succeeded
History
cc @swiatekm |
pkg/testing/runner/runner_test.go
Outdated
| stateDir := filepath.Join(tmpdir, "state") | ||
| err := os.MkdirAll(stateDir, 0755) | ||
| require.NoError(t, err) | ||
| set, err := mage.LoadSettings() |
There was a problem hiding this comment.
Nit: could we please call this settings so it doesn't sound like a function that's doing some setting work?
| fmt.Sprintf("root@localhost:%s", r.destDir), | ||
| } | ||
| cmd := exec.Command("rsync", args...) | ||
| cmd := exec.CommandContext(context.TODO(), "rsync", args...) |
There was a problem hiding this comment.
Do we have a plan to replace the TODO or should we just use Background?
What does this PR do?
Introduces a configuration struct holding all the configuration for mage targets. This configuration can be read once at the mage target level from environment variables and files, and then passed around. As a result, our local tooling code never calls
os.Setenvto pass configuration, and only callsos.Getenvwhen loading the configuration.The only place where this becomes problematic is with mage targets setting other mage targets as dependencies, but wanting to modify configuration for them. We deal with this by making all mage targets take a context parameter, and attaching the configuration to the context. Then, each mage target can try to read the configuration from context or environment, modify it, and pass it down to its dependencies.
It's worth noting that this change does not eliminate all global config from dev-tools, nor does it make all the config handling nice and clear. I'd like to address some of the remaining problems in follow-ups, here I erred on the side of making the PR reviewable.
Why is it important?
Configuring the local tooling is a mess, with environment variables being read and set all over the place. As a result, it's hard to understand how a given target behaves, and hard to test them. This is the first major step towards making this code more maintainable.
Checklist
[ ] I have made corresponding changes to the documentation[ ] I have made corresponding change to the default configuration files[ ] I have added tests that prove my fix is effective or that my feature works[ ] I have added an entry in./changelog/fragmentsusing the changelog tool[ ] I have added an integration test or an E2E testHow to test this PR locally
Use the mage targets. :) The ones involving packaging are affected most.
Related issues