Open
Conversation
swiatekm
added
skip-changelog
chore
swiatekm
commented
Feb 19, 2026
Comment on lines
-607
to
-608
| // Use mg.Deps() to ensure that the function will be called only once per mage invocation. | ||
| // devtools.Use*Packaging functions are not idempotent as they append in devtools.Packages |
Contributor
Author
There was a problem hiding this comment.
As far as I can tell, this isn't true. Both the devtools.Use*Packaging functions override a global variable, so they're idempotent. In any case, I have a follow up which will get rid of the global and make the ambiguity go away.
Contributor
💛 Build succeeded, but was flaky
Failed CI StepsHistory
cc @swiatekm |
ycombinator
reviewed
Feb 23, 2026
| if err != nil { | ||
| panic(err) | ||
| } | ||
| func MustUsePackaging(specName, specFile string, cfg *Settings) { |
Contributor
There was a problem hiding this comment.
Nit: AFAICT, cfg is being passed as the first or second argument (when ctx is the first argument). Would be good to maintain this pattern consistently.
ycombinator
reviewed
Feb 23, 2026
| actualPkgVersion, err := AgentPackageVersion(cfg) | ||
| require.NoError(t, err) | ||
| expectedPkgVersion := cfg.BeatQualifiedVersion() | ||
| actualPkgVersion := AgentPackageVersion(cfg) |
Contributor
There was a problem hiding this comment.
Nit: make this a method on cfg?
ycombinator
reviewed
Feb 23, 2026
| if err != nil { | ||
| panic(err) | ||
| } | ||
| func LoadLocalNamedSpec(name string, cfg *Settings) { |
Contributor
There was a problem hiding this comment.
Is this function being called from anywhere?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
It gets rid off a bunch of global variables containing configuration from the mage settings. These variables contained information about the repository and working directory. They were read on demand and stored to avoid recomputing them. An example is the agent version, which is read from the
version/version.gofile. They were all migrated to the Settings struct and loaded on construction. As a nice side effect, the functions no longer return errors, which saves us some boilerplate error checking.We also had some leftover code from beats supporting community beats which we don't really need, so I deleted it.
The rest of the changes involve passing Settings to functions which now need it. I've also fixed some signatures in
magefile.go, where some plain functions were reading the config from context for no good reason. Only actual mage targets should do this.Why is it important?
Keeping configuration in global variables should be avoided. It makes it difficult to test code and under
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
Related issues