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

Improve single-instance example #5829

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

Conversation

perlpunk
Copy link
Contributor

@perlpunk perlpunk commented Aug 6, 2024

It might not be clear for everyone that the file scenario-definitions.yaml has to be a local file.

I saw a question in slack regarding that.

It might not be clear for everyone that the file scenario-definitions.yaml
has to be a local file.
Copy link

github-actions bot commented Aug 6, 2024

Great PR! Please pay attention to the following items before merging:

Files matching docs/*.asciidoc:

  • Consider generating documentation locally to verify it is rendered correctly using tools/generate-docs

This is an automatically generated QA checklist based on modified files.

@@ -67,6 +67,7 @@ From there, you can trigger a new job or clone an existing one, e.g.:

[source,sh]
----
wget https://raw.githubusercontent.com/os-autoinst/os-autoinst-distri-example/main/scenario-definitions.yaml
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively we could encourage using the async=1 flag. Then openQA can download the file itself on the fly.

openqa-cli schedule --monitor \
    async=1 SCENARIO_DEFINITIONS_YAML_FILE=https://raw.githubusercontent.com/os-autoinst/os-autoinst-distri-example/main/scenario-definitions.yaml \
    DISTRI=example VERSION=0 FLAVOR=DVD ARCH=x86_64 \
    TEST=simple_boot _GROUP_ID=0 BUILD=test \
    CASEDIR=https://github.com/os-autoinst/os-autoinst-distri-example.git \
    NEEDLES_DIR=%%CASEDIR%%/needles

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any disadvantages to it?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usage: openqa-cli schedule [OPTIONS] DISTRI=… VERSION=… FLAVOR=… ARCH=… [ISO=… …]

Options:
      --apibase <path>           API base, defaults to /api/v1
      --apikey <key>             API key
      --apisecret <secret>       API secret
      --host <host>              Target host, defaults to http://localhost
  -h, --help                     Show this summary of available options
      --osd                      Set target host to http://openqa.suse.de
      --o3                       Set target host to https://openqa.opensuse.org
      --param-file <param=file>  Load content of params from files instead of
                                 from command line arguments. Multiple params
                                 may be specified by adding the option
                                 multiple times
  -m, --monitor                  Wait until all jobs are done/cancelled and return
                                 non-zero exit code if at least on job has not
                                 passed/softfailed
      --name <name>              Name of this client, used by openQA to
                                 identify different clients via User-Agent
                                 header, defaults to "openqa-cli"
  -i, --poll-interval <seconds>  Specifies the poll interval used with --monitor
  -p, --pretty                   Pretty print JSON content
  -q, --quiet                    Do not print error messages to STDERR
  -v, --verbose                  Print HTTP response headers

async=1 is not mentioned in the help and it's not immediately obvious what it does (from a user perspective).

Does it just download the file or does it do other things as well? From the code snippet it looks like it is hard-coded to always download this specific file, i.e. if I specify a different URL it will not download the file I expect?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CLI is just a client. Its help doesn't contain information about the server-side parameters. This particular flag is documented in the openQA documentation if I remember correctly.

Not sure what makes you think any paths are hardcore. Of course it'll download the file you specify.


Are there any disadvantages to it?

With the suggested change the example doesn't show anymore how to specify a local path which can also be very useful to test local modifications. Maybe we still want to have such an example somewhere in the documentation.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed the part where the change is in the docs, not the codebase, my bad 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't find any clear explanation here of what async=1 does in this case. It does download it on the server side, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Iso.pm enqueues a minion job if async is set

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the only thing that async=1 does or does it affect other things as well? i.e. if it just downloads the specified file, why is it called async? And if it has other behavior, are there any implications?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@perlpunk So the note at the bottom of https://open.qa/docs/#_spawning_multiple_jobs_based_on_templates_isos_post is not good enough? It explains what async=1 is intended to be used for.

And yes, it does not mention the SCENARIO_DEFINITIONS_YAML_FILE. Maybe we should add a note where we introduce the SCENARIO_DEFINITIONS_YAML_FILE parameter that one can specify an HTTP-URL with the async=1 parameter and that the download is then done server-side.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines +70 to 72
wget https://raw.githubusercontent.com/os-autoinst/os-autoinst-distri-example/main/scenario-definitions.yaml
openqa-cli schedule --monitor \
--param-file SCENARIO_DEFINITIONS_YAML=scenario-definitions.yaml \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to http://open.qa/docs/#_create_and_monitor_openqa_jobs_from_within_the_ci_runner
one should be able to do

SCENARIO_DEFINITIONS_YAML_FILE

Suggested change
wget https://raw.githubusercontent.com/os-autoinst/os-autoinst-distri-example/main/scenario-definitions.yaml
openqa-cli schedule --monitor \
--param-file SCENARIO_DEFINITIONS_YAML=scenario-definitions.yaml \
openqa-cli schedule --monitor \
--param-file SCENARIO_DEFINITIONS_YAML_FILE=https://raw.githubusercontent.com/os-autoinst/os-autoinst-distri-example/main/scenario-definitions.yaml \

then but something feels off here. Why would we have a parameter SCENARIO_DEFINITIONS_YAML and one SCENARIO_DEFINITIONS_YAML_FILE which can take an URL but for that we normally use the suffix _URL?

I suggest to check on that before continuing here. Just downloading a file locally to pass it unaltered to a remote server which then reads the git repo anyway sounds wrong and is not what we should encourage users to do. Maybe that's even a missing feature in openQA which I expected to be present already.

Copy link
Contributor

@Martchus Martchus Aug 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have SCENARIO_DEFINITIONS_YAML_FILE because initially it was only able to take a local file. I added the ability to specify a URL later for the webhook-based GitHub integration. And it only works if async=1 is used (which is a limitation that makes sense).

Note that SCENARIO_DEFINITIONS_YAML takes the YAML contents itself. It only works with a file here due to --param-file which is a client-side feature - which means the file needs to be downloaded on the client like in @perlpunk's wget example. (When using SCENARIO_DEFINITIONS_YAML_FILE the path would be located by the server.)

I'd probably prefer #5829 (comment). Note that your example doesn't work at all. It tells the client to lookup the local file https://raw.githubusercontent.com/os-autoinst/os-autoinst-distri-example/main/scenario-definitions.yaml and upload its contents via SCENARIO_DEFINITIONS_YAML_FILE. Also note the use of async=1 in my example.

@asmorodskyi
Copy link
Member

I think it worth to mention that this flow is also used in osado CI https://github.com/os-autoinst/os-autoinst-distri-opensuse/blob/master/.github/workflows/openqa.yml#L37

@okurz
Copy link
Member

okurz commented Aug 6, 2024

I think it worth to mention that this flow is also used in osado CI https://github.com/os-autoinst/os-autoinst-distri-opensuse/blob/master/.github/workflows/openqa.yml#L37

Yes. No problem there as the CI job has access to the git repository checked out with the file scenario-definitions.yaml within

@asmorodskyi
Copy link
Member

and how SCENARIO_DEFINITIONS_YAML_FILE relates to YAML_SCHEDULE ? 🤔 isn't it the same thing ?

@Martchus
Copy link
Contributor

Martchus commented Aug 6, 2024

No, it is a completely different thing. The only commonality is that both features are using YAML.

Scenario definitions allow you to define a set of jobs to run by specifying machines, archs, products, testsuites and so on.

The "YAML schedule" is an osado convention/feature for deciding what test modules to run for a certain test scenario.

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.

6 participants