Skip to content

Conversation

@nupurg-ibm
Copy link
Collaborator

@nupurg-ibm nupurg-ibm commented Oct 9, 2024

Description

Release required?

  • No release
  • Patch release (x.x.X)
  • Minor release (x.X.x)
  • Major release (X.x.x)
Release notes content

Run the pipeline

If the CI pipeline doesn't run when you create the PR, the PR requires a user with GitHub collaborators access to run the pipeline.

Run the CI pipeline when the PR is ready for review and you expect tests to pass. Add a comment to the PR with the following text:

/run pipeline

Checklist for reviewers

  • If relevant, a test for the change is included or updated with this PR.
  • If relevant, documentation for the change is included or updated with this PR.

For mergers

  • Use a conventional commit message to set the release level. Follow the guidelines.
  • Include information that users need to know about the PR in the commit message. The commit message becomes part of the GitHub release notes.
  • Use the Squash and merge option.

@nupurg-ibm nupurg-ibm self-assigned this Oct 9, 2024
@nupurg-ibm
Copy link
Collaborator Author

/run pipeline

@nupurg-ibm nupurg-ibm requested a review from ocofaigh October 9, 2024 18:10
}
network_acl_inbound_rules = [
{
name = "test-1"
Copy link
Contributor

@daniel-butler-irl daniel-butler-irl Oct 14, 2024

Choose a reason for hiding this comment

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

These will be real rules on customer accounts, right? You probably shouldn't use the name test. It would be better to use a full description of the rule for the name

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we can rename it

Copy link
Contributor

Choose a reason for hiding this comment

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

@daniel-butler-irl As discussed we shall change the name here

chmod +x install-opentofu.sh

# Run the installer:
./install-opentofu.sh --install-method rpm
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like you are installing Opentofu in order to use it with modules from TIM. This will not work, it might work today but we are already seeing the two versions diverge and I expect within the next year or two there will be incompatibilities. For example, it is possible to write OpenTofu configuration that is not compatible with Terraform today.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We actually needed opentofu to run the testcases. If any alternative way is suggested let that know.

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be a bug in the testwrapper causing the requirement. Updating the wrapper to see if it fixes the requirement.

Copy link
Contributor

Choose a reason for hiding this comment

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

@daniel-butler-irl As we discussed, we shall upgrade the required versions and remove the tofu installations and try if that works..

And did you mean with the existing version what we are using is having a Bug ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I expect it's a bug, hopefully, it is resolved in a later version. We have not seen the bug before so I am hoping it got resolved as a side effect of other refactoring work. That version is over a month old and no reports of this bug.

git clone --depth=1 https://github.com/tfutils/tfenv.git ~/.tfenv
echo "export PATH=$PATH:$HOME/.tfenv/bin" >> ~/.bashrc
ln -s ~/.tfenv/bin/* /usr/local/bin
tfenv install 1.5.7
Copy link
Contributor

@daniel-butler-irl daniel-butler-irl Oct 16, 2024

Choose a reason for hiding this comment

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

Do you have a plan for maintaining the versions installed in this script? For instance, we are already on terraform version 1.9.x

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean how do you keep the versions in this script updated?

Copy link
Contributor

Choose a reason for hiding this comment

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

@daniel-butler-irl Good Point.. We can use the latest instead of pointing to the specific version.

We did a quick check and we came up with using the below:
tfenv install latest
tfenv use latest

Which shall install and use the latest version. For an example when I ran through it picked up 1.9.7

[root@sagar-compute-rhel810 ~]# tfenv install latest Installing Terraform v1.9.7 Downloading release tarball from https://releases.hashicorp.com/terraform/1.9.7/terraform_1.9.7_linux_amd64.zip ############################################################################################################################################################################################################################################## 100.0% Downloading SHA hash file from https://releases.hashicorp.com/terraform/1.9.7/terraform_1.9.7_SHA256SUMS Not instructed to use Local PGP (/root/.tfenv/use-{gpgv,gnupg}) & No keybase install found, skipping OpenPGP signature verification perl: warning: Setting locale failed. perl: warning: Please check that your locale settings: LANGUAGE = (unset), LC_ALL = (unset), LC_CTYPE = "UTF-8", LANG = "en_US.UTF-8" are supported and installed on your system. perl: warning: Falling back to a fallback locale ("en_US.UTF-8"). Archive: /tmp/tfenv_download.QFN1LC/terraform_1.9.7_linux_amd64.zip inflating: /root/.tfenv/versions/1.9.7/LICENSE.txt inflating: /root/.tfenv/versions/1.9.7/terraform Installation of terraform v1.9.7 successful. To make this your default version, run 'tfenv use 1.9.7'

[root@sagar-compute-rhel810 ~]# tfenv use latest Switching default version to v1.9.7 Default version (when not overridden by .terraform-version or TFENV_TERRAFORM_VERSION) is now: 1.9.7 [root@sagar-compute-rhel810 ~]# terraform --version Terraform v1.9.7 on linux_amd64

Hope this is as expected !

@nupurg-ibm
Copy link
Collaborator Author

/run pipeline

@daniel-butler-irl
Copy link
Contributor

@nupurg-ibm @bksagar I nearly missed it but you are going to need a test for this. I would put it in the other_test.go so it runs weekly. I am recommending this because not all your versions are pinned so you are running on untested versions. At least if you are running a weekly test you will be notified if something breaks. You should also consider how you plan on maintaining the versions you do have pinned in the scripts so you are not providing customers with versions that contain critical security vulnerabilities. The rest of the PR looks good to me, I won't block you from merging this but I strongly advise you implement at least one test...

@ocofaigh
Copy link
Contributor

@daniel-butler-irl This repo is not set up with a continuous test pipeline, so anything in other_test.go will never get executed unless its added

@nupurg-ibm
Copy link
Collaborator Author

@daniel-butler-irl, could we please get your approval on this? We can discuss later if tests are necessary for this and how to integrate them with the custom-image-builder

@nupurg-ibm nupurg-ibm changed the title adding custom image builder code base Adding custom image builder functionality Oct 21, 2024
@nupurg-ibm nupurg-ibm changed the title Adding custom image builder functionality Custom image support for HPC solution Oct 21, 2024
@nupurg-ibm nupurg-ibm merged commit 287bf72 into main Oct 21, 2024
2 checks passed
@nupurg-ibm nupurg-ibm deleted the 9-oct branch October 21, 2024 14:43
@terraform-ibm-modules-ops
Copy link
Contributor

🎉 This PR is included in version 1.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants