Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor into stateful types and remove viper dependency from pkg/ code #181

Merged
merged 9 commits into from
Apr 28, 2024

Conversation

xrstf
Copy link
Contributor

@xrstf xrstf commented Apr 18, 2024

This started out as a simple "remove the viper.WriteConfig line" PR and then slowly escalated over the last few hours.

My main goal here was to fix #175 and my idea was to introduce a Configuration struct that we can pass into othe components, instead of making the entire codebase rely on global state and viper.String() calls.

However, mapping pflags via viper to a config file makes it, as far as I could see, impossible to keep the Configuration struct I was aiming for. When using viper, the whole "load the config file, merge it with CLI flags" logic is based around the idea that you then use viper.Get... to retrieve the merged data. Viper will not (AFAIK) write the merged data back into the struct.

So instead of using Viper to map the flags, I hand rolled a bit of code to load a YAML file and merge the flags with it. As discussed in #175, the logic is:

  • if something is explicitly given via CLI flag, that has priority.
  • if not, if a config file was given explicitly via --config, it is loaded and the value in the config file is used if not empty.
  • if there is no config file or the value in the config file is empty, then the hydrophone default value is used.

The config file is never written to by hydrophone. Users have to manually create one if they want to use it.

Importantly, the 3 different run modes (cleanup, list-images, conformance[/--focus]) are not part of the Configuration, as they only serve a purpose in the root command to distinguish which code to call. Personally I would even remove Verbosity, DryRun and Parallel from the config file, but it doesn't hurt to have them there either.


Once the config was turned into a struct, I proceeded to get rid of some init() functions to reduce overall global state. What previously were global static functions like service.Cleanup are now functions on a TestRunner struct, which holds the necessary dependencies (and, importantly, gets the Configuration injected).

The service package was renamed to conformance, as the package implements running the Kubernetes conformance suite. The client to access the results was moved to pkg/conformance/client.

Lots of code from the common package was moved into a new types package, type-ical for Kubernetes projects. 😁

The recently introduced ImagePullBackoff check was moved into a CreatePod() helper function that now simply waits for the Pod to be running before continuing.


One thing lead to another and now half the codebase was moved around 😬

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 18, 2024
@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Apr 18, 2024
@xrstf xrstf force-pushed the explicit-config branch 2 times, most recently from fd4bf4e to 7a7a739 Compare April 19, 2024 09:48
@xrstf
Copy link
Contributor Author

xrstf commented Apr 19, 2024

/kind cleanup

@k8s-ci-robot k8s-ci-robot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Apr 19, 2024
@xrstf xrstf changed the title WIP - refactor into stateful types and remove viper dependency from pkg/ code Refactor into stateful types and remove viper dependency from pkg/ code Apr 19, 2024
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 19, 2024
@xrstf xrstf changed the title Refactor into stateful types and remove viper dependency from pkg/ code WIP - Refactor into stateful types and remove viper dependency from pkg/ code Apr 19, 2024
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 19, 2024
@xrstf xrstf changed the title WIP - Refactor into stateful types and remove viper dependency from pkg/ code Refactor into stateful types and remove viper dependency from pkg/ code Apr 19, 2024
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 19, 2024
@rakshitgondwal
Copy link
Member

Thanks for this, it is going to take a little while to review this whole, but even I was thinking what could be the best way to fix the issue with viper.
One thing I thought about was to just use WriteConfig() instead of SafeWriteConfig() which would lead to an overwrite of config file everytime. But then another thing came up that do we actually need Viper? why don't we just make the flag variables as global and reference them across, not sure if this would be a tidy method though.

But still I'll have a look at your approach :)

@xrstf
Copy link
Contributor Author

xrstf commented Apr 23, 2024

why don't we just make the flag variables as global and reference them across

Global variables make it extremely hard to understand data flow and make it uneccessarily complicated to re-use / test code. Calling viper.String() everywhere is just as bad as having a global variable. Hydrophone's code was already sprinkled with init() functions that made it complicated to follow what's going on. I don't think adding even more global state would help the codebase.

One thing I thought about was to just use WriteConfig() instead of SafeWriteConfig() which would lead to an overwrite of config file everytime.

And what is the point then to overwrite the config file everytime? In what scenario would that be desirable? If I don't specify a --config flag, I do not want to use a config file. If I do specify the --config flag, then please do not fiddle with it and overwrite it. :D I might have mounted it from a ConfigMap or run in a read-only filesystem.

Even if you overwrite the config file every single time, you are still remembering the last settings/flags and re-using them for the next run of hydrophone. So we would still diverge from what is shown as default in --help and what is actually the "default".

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 27, 2024
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 27, 2024
@rjsadow
Copy link
Collaborator

rjsadow commented Apr 28, 2024

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 28, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rjsadow, xrstf

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 28, 2024
@k8s-ci-robot k8s-ci-robot merged commit 7eb00e2 into kubernetes-sigs:main Apr 28, 2024
8 checks passed
@xrstf xrstf deleted the explicit-config branch April 28, 2024 07:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Confusing configuration file
4 participants