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 playbook for openqa pt install #247

Merged
merged 1 commit into from
Jun 27, 2024
Merged

Conversation

BillAnastasiadis
Copy link
Collaborator

@BillAnastasiadis BillAnastasiadis commented Jun 24, 2024

This pr adds

@BillAnastasiadis BillAnastasiadis force-pushed the ptf2 branch 2 times, most recently from 97561bf to d6b2ee6 Compare June 24, 2024 13:53

- name: Extract the first part of az_container_name
set_fact:
az_container_top: "{{ az_container_name.split('/')[0] }}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

we can avoid this "trick" (that create hidden dependency of path used in hana_media.yaml and playbooks/sap-hana-download-media.yaml) and just force the user to provide the path in a second variable just for this playbook

ptf_installation.yaml -e ptf_container="aaa/bbb/ptf" -e ptf_files="aaa.rpm,bbb.rpm"

or with the path in each file element

ptf_installation.yaml  -e ptf_files="aaa/bbb/ptf/aaa.rpm,aaa/bbb/ptf/bbb.rpm"

Copy link
Collaborator Author

@BillAnastasiadis BillAnastasiadis Jun 25, 2024

Choose a reason for hiding this comment

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

Yes, that can be done - but is it really needed? This code just grabs the first part, which is always bound to be the name of the top-level container, which isn't going to change (or, if it changes, it will change for everything). Let's discuss in slack

Copy link
Collaborator

Choose a reason for hiding this comment

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

We can decide to use a dedicated container for PTF.
Maybe one with a subfolder for each PTF

@BillAnastasiadis BillAnastasiadis force-pushed the ptf2 branch 3 times, most recently from 005142d to d7ceafc Compare June 25, 2024 11:03
@BillAnastasiadis BillAnastasiadis changed the title WIP: Add playbook for openqa pt install Add playbook for openqa pt install Jun 27, 2024
ptf_url: "your_ptf_url"
ptf_user: "your_username_here"
ptf_password: "your_pass_here"
url_timeout: 30
Copy link
Collaborator

Choose a reason for hiding this comment

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

why we like them to be parametric?
why hardcoding value directly in Download PTF files with SAS token is not enough?

Copy link
Collaborator

@mpagot mpagot left a comment

Choose a reason for hiding this comment

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

LGTM

My only concern is that it come out with a complicated API (no idea if /how we can improve it)

  • url_timeout, url_retries_cnt, url_retries_delay can be configured from the external but has default. There's no validatio nof values if configured from the external.
  • sas_token is optional. Its presence is one of the variables checked to decide which download task to use. Name overlap with the one used in hana_media.yaml but they are two different variables (even if for the moment, due to what we currently have in Azure, token is the same). It cold be an idea to rename it to ptf_sas_token and just externally (openQA) use same token used for HANA install?
  • ptf_files is mandatory if sas_token is defined. It has to have a special format like comma separated list of filename (or is filename + some path?). Its presence is not checked or validated before to start using it
  • container, storage are mandatory if sas_token is defined. They have to be strings. Their presence are not checked or validated before to start using them
  • ptf_user, ptf_password, ptf_url are optional and used to select a specific download task. The first two are string, password is a secret, ptf_url is an url. None are validated.

@BillAnastasiadis
Copy link
Collaborator Author

LGTM

My only concern is that it come out with a complicated API (no idea if /how we can improve it)

  • url_timeout, url_retries_cnt, url_retries_delay can be configured from the external but has default. There's no validatio nof values if configured from the external.
  • sas_token is optional. Its presence is one of the variables checked to decide which download task to use. Name overlap with the one used in hana_media.yaml but they are two different variables (even if for the moment, due to what we currently have in Azure, token is the same). It cold be an idea to rename it to ptf_sas_token and just externally (openQA) use same token used for HANA install?
  • ptf_files is mandatory if sas_token is defined. It has to have a special format like comma separated list of filename (or is filename + some path?). Its presence is not checked or validated before to start using it
  • container, storage are mandatory if sas_token is defined. They have to be strings. Their presence are not checked or validated before to start using them
  • ptf_user, ptf_password, ptf_url are optional and used to select a specific download task. The first two are string, password is a secret, ptf_url is an url. None are validated.
  • this was inherited from sap_download-hana-media.yaml, which uses them in the Download HANA media with SAS token task. Since the sas-token method of downloading in my playbook is basically copied from that function, I used the same args and values.
  • sas_token does not have the same name as in sap_download-hana-media.yaml: there, it's az_sas_token, while I renamed it to simply sas_token
  • I can validate it, should I try? Or should we let the task fail and the reviewer review it?
  • Similar question as above
  • Similar as above

Copy link
Collaborator

@lpalovsky lpalovsky left a comment

Choose a reason for hiding this comment

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

LGTM

@mpagot mpagot merged commit 607a7e6 into SUSE:main Jun 27, 2024
1 check 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