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

Add arguments for komodo-transpiler #308

Closed
xjules opened this issue Jan 18, 2023 · 8 comments · Fixed by #315
Closed

Add arguments for komodo-transpiler #308

xjules opened this issue Jan 18, 2023 · 8 comments · Fixed by #315
Assignees

Comments

@xjules
Copy link
Contributor

xjules commented Jan 18, 2023

Currently komodo-transpiler transpile has hard-coded arguments for specifying python and rhel versions. We should be able to retrieve the versions from the cli arguments.
For example:

komodo-transpiler transpile
        --matrix-file=releases/matrices/${{ inputs.target }}.yml
        --python-ver=3.8
        --rhel-ver=7
        --output releases
@kwinkunks
Copy link
Member

kwinkunks commented Jan 18, 2023

Just a question because as mentioned in the stand-up, I find the language a bit confusing.

❓ Does the Release Manager want to maintain a single release matrix file, or multiple release files? I think it's the matrix file, is that correct?

(I know right now that the matrix file and release file are "sort of" the same thing, although strictly speaking what is being maintained is a release file.)

I see that komodo-transpiler can go either way:

  • The combine command takes multiple separate release files and creates a single matrix file.
  • The transpile command takes a single matrix file and splits it into multiple release files.

@kwinkunks
Copy link
Member

I had a thought about how to add the urgently needed capability to produce a single 'cell' in a release matrix, without having to hardcode 'redhat' and 'python' into the tool in yet another place.

Maybe the interface could look like:

komodo-transpiler transpile
    --matrix-file=releases/matrices/${{ inputs.target }}.yml
    --job py38,rhel7
    --output releases

This --job argument can then be split into the two 'coordinates' of the matrix. We are free to interpret them however, but I think the tool only needs to care that they are 'thing 1' and 'thing 2', right?

Maybe release_transpiler.py can also be refactored to not use variable names like rhel_ver and py_ver, but I didn't look at how hard that would be. And eventually we will want it to allow for an arbitrary number of 'coordinates' in job but we can deal with that when we need it.

What do you think @xjules and @hnformentin? Would this work?

@hnformentin
Copy link
Contributor

hnformentin commented Jan 19, 2023

Thanks for the comments.

  • In which cases will we only want to release one combination from the matrix? Will we have situations where we want two of them Helping to considerate @kwinkunks suggestion to use --job entry
    Comment: we want to use transpile functionality in github actions. It is useful to support lists of versions, though the main use case will be a single os and a single python version. The idea of -job is useful and indeed cleaner, but it seems that a more explicit format with two independent entries would be more readable.

  • Which are possible places other than komodo where the hard-corded variables RH-Versions and PY_VERSIONS are used? Helping to answer if we can completely remove this hardcoded variables
    Comment: As I understood from a chat with @xjules, removing residuals of Jenkins will be a next milestone, so when we are sure we don't have unexpected side effects, we can remove this functionality. Also, as a public library, we should first show a deprecation warning for this default values.

The point that @kwinkunks raised is valid, but let's treat it on another issue #316.

@hnformentin
Copy link
Contributor

hnformentin commented Jan 20, 2023

Some changes to consider after discussion.
This are examples that should work:

(1) Base case:

komodo-transpiler transpile
        --matrix-file=release.yml
        --output releases
        rhel=7
        py=3.8

(2) Should be supportable in the future:

komodo-transpiler transpile
        --matrix-file=release.yml
        --output releases
        macos=1
        py=3.8

(3) Use default values rhel=7 and py=3.8:

komodo-transpiler transpile
        --matrix-file=release.yml
        --output releases

We discussed using nargs="*" and as far as I understood the = depends on design option.

A requirement is to validate the input version within the repository.yml file in komodo-releases.
The depth for the packages in terms of number of versions should support this validation.

@kwinkunks
Copy link
Member

I feel like we should not support option (3) unless we absolutely have to.

For the mapping, I think it should be possible to make a CLI call like:

komodo-transpiler transpile
    --matrix-file foo.yml
    --output releases
    --coords {py: "3.8", rhel: "7"} 

That's YAML formatting... could also use JSON formatting, but I think it needs more quotes, like {"py": "3.8", "rhel": "7"}.

You can do this sort of thing like:

parser.add_argument('--coords', type=yaml.safe_load, help='coords of matrix element')

The nice thing about this is you get a dictionary which has arbitrary keys and values, so you avoid having to pick it apart to get at everything, and it's a bit more future proof because I can pass in macos: "10.15" if I want.

For this example the result is:

argparse.Namespace(coords={'py': '3.8', 'rhel': '7'})

If you stick with py=3.8 rhel=7 I think I would probably still add an argument name like --coords or something to make it clear what's being passed.

Again, I definitely might be missing something about how exactly this needs to be called!

@hnformentin
Copy link
Contributor

I think the problem with passing --coords in yml format is that github actions inputs only support string, choice, boolean and environment.
We want to take these komodo arguments as input from github actions. The way to access one of the inputs would then be:

komodo-transpiler transpile 
        --matrix-file=release.yml
        --output=releases
        rhel=github.events.input.rhel
        py=github.events.input.py 

Would the following be the alternative proposed?

komodo-transpiler transpile
    --matrix-file foo.yml
    --output releases
    --coords {py: github.events.input.py, rhel: github.events.input.rhel} 

The problem with not having the default values is backwards compatibility. If we want to remove the default values, I think we should first have a deprecation warning and after sometime remove the default.

@kwinkunks
Copy link
Member

I prefer the second pattern, if it gets the job done. I agree existing defaults should be deprecated (some time, not necessarily now), and only suggest not adding new defaults that specify architecture or other 'coordinates'.

@hnformentin
Copy link
Contributor

Thanks for persisting! That makes sense and, with @lars-petter-hauge, we found out that it is also possible to implement as a list.
This will be the way I will try to implement:

komodo-transpiler transpile
    --matrix-file foo.yml
    --output releases
    --coords {py: [ ${{ inputs.python_version }} ], rhel: [ ${{ inputs.rhel_version }} ]} 

where inputs.python_version is a string (e.g. 3.8) or a string of comma-separated items (e.g. 3.8, 3.10).

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

Successfully merging a pull request may close this issue.

3 participants