Allow using archive path for ocp4-helpernode and ocp4-playbooks

[torwen1]

Fixes #220

Implementation how I did it at another customer. I have tested it again on our local test env and both (git and curl) was working like it should be.

I also added a helm_repo URL to the var.tfvars (as long the package is still needed), so that users can also host this package on their own environment.

[ppc64le-cloud-bot]

Hi @torwen1. Thanks for your PR.

I'm waiting for a ocp-power-automation member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[yussufsh]

I have following points to make on the overall patch:

1. Instead of .tar.gz or .tgz we can use the extension .zip. This format is directly provided by GitHub UI to download the zip. You don't require extra steps to archive, just download the branch you want to use and place it on a web server.
2. Instead of adding prefix as git: then removing from the code, we should use the extension/suffix of the URL. If ends with .zip download it and extract OR assume it is a git repo by default, do clone and checkout.
3. The archived file might have the code directory that is not a git repo, no need to checkout a tag (git checkout I assume will need internet connection).
4. We can remove the trigger on the worker count as worker changes don't need updated helpernode or install code. (This can lead to confusion on what code level the cluster was initially installed). I am okay keeping it as well.
5. We need to ensure curl and unzip are installed on bastion/helpernode.

[torwen1]

Thanks a lot for your input.
Regarding your points:

1. This is what I did at the beginning at the customer. Downloaded it as a ZIP file, which contains the HEAD code. But in the helpernode.tf and install.tf the code explicitly does a checkout for a specific tag (probably to ensure that tested versions of helpernode and playbooks are used). We would loose this function and the users have to take care of to download the correct version. Another issue I found with the ZIP Download is: The file name contains -master and if you extract it, the folder name doesn't match. That's the reason why I stopped following this approach.
2. Also a good option. Will change it this way.
3. I just took over the "checkout tag" from the original code. For me it looks like that it shall ensure, that we use the a tested version of helpernode and playbooks. Is it no longer needed and just something from the past? The checkout against a clone will work locally without internet access (this is how I understood it, because a clone is a copy of the repository with its own history, logs, etc...).
4. I wasn't 100% sure what the purpose of the trigger is, but with your explanation I understand it better. I can remove the triggers for both new resources in helpernode.tf and install.tf.
5. Good point. I will add this stuff in the code.

[yussufsh]

1. This is what I did at the beginning at the customer. Downloaded it as a ZIP file, which contains the HEAD code. But in the helpernode.tf and install.tf the code explicitly does a checkout for a specific tag (probably to ensure that tested versions of helpernode and playbooks are used). We would loose this function and the users have to take care of to download the correct version. Another issue I found with the ZIP Download is: The file name contains -master and if you extract it, the folder name doesn't match. That's the reason why I stopped following this approach.

We could always automate folder rename. Users can clone and zip/tar by downloading it on a machine with internet access or use the Github UI to download the branch. We can leave it up to the user and have this documented. TF will download the archive and extract it to a folder and run it as an ansible project.

Also for point 3, no need for a checkout command if the path is an archive. For git repos we should run checkout.

[torwen1]

Hi, have updated the code and squashed the commits into one:

1. I swapped from tar.gz to zip with the folder structure in the zip like you download it from the GitHub website (-master at the end of the extracted folder). I added a mv to rename the folder e.g. ocp4-helpernode-master to ocp4-helpernode and updated the documentation accordingly, that we expect this folder structure in the ZIP file, incl. the recommended way to download it.
2. Removed the git tag. So, the pattern will check for URL ending with .zip and decides which way to go.
3. Removed the work counter triggers in helpernode.tf and install.tf. helpernode.tf still contains the trigger for the bootstrap (that's how I understood what was asked for)
4. added in the bastion.tf, that curl and unzip will be installed, if it is missing.

[yussufsh]

@Prajyot-Parab can you or someone else test this feature out? Need to make sure this works fine before accepting the change.

[aishwaryabk]

The code works fine. However, the ocp4-helpernode releases on GitHub are not appropriate for this code. As the zipped file extraction does not have the ansible files format.

[torwen1]

@aishwaryabk Thanks a lot for testing. You are right regarding the release versions of ocp4-helpernode.
With this project we always pulled from the master repo.

My suggestion is to mentioning in the documentation that the ZIP must be created from the master repository and not using the releases. That's how I would "solve" this topic. What do you think?

[aishwaryabk]

@aishwaryabk Thanks a lot for testing. You are right regarding the release versions of ocp4-helpernode.
With this project we always pulled from the master repo.

My suggestion is to mentioning in the documentation that the ZIP must be created from the master repository and not using the releases. That's how I would "solve" this topic. What do you think?

IMO this should be fine.

[torwen1]

I have updated the var-tfvars.md and described that the download needs to be done from the master repository and removed the part with selecting it from the release tab.

[yussufsh]

@torwen1 can we not support any branch? It should be possible to give the target file name while un-archiving the zip. So any download from Github GUI should be supported IMO.

[torwen1]

@yussufsh Sure, technically it should work. I just thought that it would be the easiest for the users to have one documented way, instead of option a, b, c. But I can simply update the documentation with", or any other supported branch".

[torwen1]

I have updated the documentation, that users can also select any supported branch for downloading the ZIP package, instead just from the master repo.

[torwen1]

@yussufsh Just to pop up the merge request again :)

[yussufsh]

/lgtm

[ppc64le-cloud-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: torwen1, yussufsh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [yussufsh]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[ppc64le-cloud-bot]

New changes are detected. LGTM label has been removed.

[torwen1]

Creating new merge request because with this one I cannot solve the merge conflicts.