Separate intrinsic environment variables - #5054
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a separation between user-provided and tmt-owned ("intrinsic") environment variables by adding the HasIntrinsicEnvironment abstract base class and refactoring relevant classes (such as Plan, Guest, TestInvocation, and various execution contexts) to inherit from it and expose an intrinsic_environment property. This ensures that tmt-owned variables consistently overwrite user-provided variables. Additionally, Environment.update is simplified to accept only Environment instances. There are no review comments, and I have no feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
4268aa0 to
c777740
Compare
LecrisUT
left a comment
There was a problem hiding this comment.
I wonder if we can have a more generic way to define this sources. I've hit a similar design issue at scikit-build-core. Basically we need to manage a list of list of environment (outer list of user vs intrisic, inner one from each source)
I'm pretty happy with the state of things I reached. Maybe there will be a chance to build something sleaker once envvars become objects rather than just names as strings, but at this point of environment cleanup/secrets bootstrap, I'm perfectly fine with the state of cleanup, and I don't want to dive into more refactoring. |
tmt-owned variables are no longer included in the
`HasEnvironment.environment` return values. Separating the user-owned
and tmt-owned variables will give us precise control over their
ordering.
Patch is also dropping support of `Environment.update(foo: HasEnvironment)`
shortcut - an actual `Environment` instances are required now. Once a
class offers both user-provided and tmt-owned variables, `update()`
would have hard time to pick which set to include in the target
environment ("both" might also be an answer...).
This should be the penultimate patch of the environment precedence
series; the next and almost-final patch will provide unified helper to
construct environments so they follow the same process.
Related to #2609.
dfb2ecb to
db4edb8
Compare
tmt-owned variables are no longer included in the
HasEnvironment.environmentreturn values. Separating the user-owned and tmt-owned variables will give us precise control over their ordering.Patch is also dropping support of
Environment.update(foo: HasEnvironment)shortcut - an actualEnvironmentinstances are required now. Once a class offers both user-provided and tmt-owned variables,update()would have hard time to pick which set to include in the target environment ("both" might also be an answer...).This should be the penultimate patch of the environment precedence series; the next and almost-final patch will provide unified helper to construct environments so they follow the same process.
Related to #4241.
Pull Request Checklist