feat: replace hardcoded artifact image constants with CLI-configurable values #322
Conversation
Alex619829
changed the title
Alex619829
force-pushed
the
main
branch
4 times, most recently
from
03265ce to
2aecfd0
Compare
|
Hello! Is this PR ready for review? There's some kind of error in the workflow, I don't know what to do with it. |
|
the workflow won't run unless one of the maintainers reviews and approves the workflow, this will be looked at probably next week |
Okay, thank you! |
cmd/image-factory/cmd/options.go
Outdated
| SecureBoot SecureBootOptions | ||
|
|
||
| // Organization or platform within an organization. | ||
| Organization string |
There was a problem hiding this comment.
let's remove this please, it makes things too complicated
There was a problem hiding this comment.
I have fixed it at the last commit
cmd/image-factory/cmd/options.go
Outdated
| TalosctlImage string | ||
|
|
||
| // Path to YAML file with extension name aliases. | ||
| ExtensionNameAlias string |
There was a problem hiding this comment.
please split this into separate PR
internal/artifacts/artifacts.go
Outdated
|
|
||
| // Various images. | ||
| const ( | ||
| var ( |
There was a problem hiding this comment.
these should stay as constants/default values, or be removed completely. setting variables via config is a bad idea
There was a problem hiding this comment.
Okay, I won't touch it for now.
There was a problem hiding this comment.
Clarifying myself: patching variables in a module is a bad development practice.
We can keep those as constants and initialize config values from it (or skip it).
But we should use config values where we previously used these constants.
Alex619829
changed the title
Alex619829
changed the title
smira
left a comment
smira
left a comment
There was a problem hiding this comment.
this seems non-functional now? as these config values are not consumed anywhere?
Aren't we passing parameters in this line to the main.go file: |
we do, but nothing "reads" from these new variables/options unless I'm missing something |
Ah, okay, I get it. This brings us back to the point that the most convenient way to use these values in code is to replace constants with variables and change them using these parameters. That's exactly why I want to add flags. |
|
No, please - we need to pass config options to the places where it's going to be consumed. |
Okay, undestand you. |
…e values Adds CLI flags that replace hardcoded image constants Signed-off-by: Aleksandr Gamzin <[email protected]> Signed-off-by: Andrey Smirnov <[email protected]>
smira
left a comment
smira
left a comment
There was a problem hiding this comment.
thank you, I did some changes to the PR
|
/m |
Thank you! |
4 participants
This pull request adds new flags that replace hardcoded image constants. These are needed to ensure flexibility and reusability of image-factory across different environments and organizations.