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 python cmdline parser for variants #180

Merged
merged 5 commits into from
Sep 25, 2024

Conversation

Vincent-lau
Copy link
Contributor

Given

type operation = Copy of string | Mirror of string

Represented as ["Copy", "arg"] we need to parse that as JSON when given on the cmdline.

Otherwise it'd do operation[0], where operation is a string, which is not what we want.

This doesn't affect usage from xapi-storage-script (which always calls it with --json),
but it affects testing the python code by hand, because you can't supply the correct cmdline arguments
(unless you construct an entire JSON by hand and pass it on stdin which is a bit tedious)

@edwintorok
Copy link
Collaborator

Hmm the CI seems to be broken

edwintorok and others added 2 commits September 24, 2024 14:22
Given
```
type operation = Copy of string | Mirror of string
```

Represented as `["Copy", "arg"]` we need to parse that as JSON when given on the cmdline.

Otherwise it'd do `operation[0]`, where `operation` is a string, which is not what we want.

This doesn't affect usage from xapi-storage-script (which always calls it with --json),
but it affects testing the python code by hand, because you can't supply the correct cmdline arguments
(unless you construct an entire JSON by hand and pass it on stdin which is a bit tedious)

Signed-off-by: Edwin Török <[email protected]>
@Vincent-lau
Copy link
Contributor Author

  /Users/runner/hostedtoolcache/opam/2.2.1/arm64/opam switch --no-install --packages=ocaml-base-compiler.4.08.1 create .

Looks like it is running 4.08.1 on macos, is it supposed to do that?

@mseri
Copy link
Collaborator

mseri commented Sep 24, 2024

I think we should probably update setup-ocaml to the latest version

uses: ocaml/setup-ocaml@v2

@Vincent-lau
Copy link
Contributor Author

Vincent-lau commented Sep 25, 2024

The previous issue was due to pylint complaining about the possibly uninitialised variable unicode, which is only set to str when we are on python3. But we are not testing this on python2 anyway, so I dropped the guarding if

@edwintorok
Copy link
Collaborator

We've migrated XAPI and everything to Python3 recently, so I'm fine with dropping python2 support.

Since we are not testing it in the CI anyway.
@edwintorok edwintorok merged commit d558f6a into mirage:master Sep 25, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants