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

Fix for bundle version in the installer CLI #449

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

georgievaVMW
Copy link
Contributor

Made small changes so the CLI shows correct bundle version.
Made a map from k8s filter to k8s bundle (this is in order to easily output correct bundle for example 1.23.* -> 1.22)
Made changes to installer.go and the registry tests to accommodate the added second argument to the function.

Made a map from k8s filter to k8s bundle (this is in order to easily output correct bundle for example 1.23.* -> 1.22)
Made changes to installer.go and the registry tests to accomodate the added second argument to the function.
@codecov-commenter
Copy link

Codecov Report

Merging #449 (9db95f3) into main (cc605cf) will decrease coverage by 0.00%.
The diff coverage is 80.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #449      +/-   ##
==========================================
- Coverage   68.40%   68.39%   -0.01%     
==========================================
  Files          23       23              
  Lines        1728     1734       +6     
==========================================
+ Hits         1182     1186       +4     
- Misses        474      476       +2     
  Partials       72       72              
Impacted Files Coverage Δ
agent/installer/registry.go 96.66% <71.42%> (-3.34%) ⬇️
agent/installer/installer.go 78.10% <100.00%> (ø)

@jeuxdemains
Copy link
Contributor

A note on why we switched from filter array to filter map.
We had a discussion with Alex about this update and the path he can take to implement it.

Initially the installer was meant to use a simple container (array) to hold the filters and that was the right choice at the time.
However, the CLI require certain level of predictability in order to perform as expected (to print the supported versions) which is not possible without predefining to which K8s versions the filters match.

We discussed different solutions like stripping the filter masks (the dot asterisk part) from the string of the help of the CLI, as well as extracting the map to separate module which could be shared between modules (CLI and Installer).
Ultimately we concluded integrating it to the Installer as a map would cost less time and be better prepared for the future, requiring less portions of the code to be maintained.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants