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

Trillian extend - createTree and updateTree #39

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

ssulka
Copy link
Contributor

@ssulka ssulka commented Jan 30, 2025

No description provided.

@ssulka ssulka requested review from osmman and bouskaJ January 30, 2025 12:19
Copy link
Contributor

@osmman osmman left a comment

Choose a reason for hiding this comment

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

Please resolve conflicts


Describe("Set tree state to DRAINING", func() {
It("should update tree state", func() {
cmd := fmt.Sprintf("oc run --image registry.redhat.io/rhtas/updatetree-rhel9@sha256:1a95a2061b9bc0613087903425d84024ce10e00bc6110303a75637fb15d95d34 --restart=Never --attach=true --rm=true -q -- updatetree --admin_server=trillian-logserver:8091 --tree_id=%s --tree_state=DRAINING", currentTreeID)
Copy link
Contributor

Choose a reason for hiding this comment

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

First, avoid hard coding any version of the tested artifact binary/image in the test suite. Hard coding versions undermines the verification process.

Next, please use the same method as is used for other CLI tools to download the updatetree and createtree binaries. This ensures consistency and reliability across the codebase, and it is essential for all tests in this repository to work seamlessly on both OCP (OpenShift Container Platform) and RHEL (Red Hat Enterprise Linux) environments. The current implementation does not meet this requirement.

})
})

Describe("Set tree state to DRAINING", func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Additionally, please do not manipulate the existing tree because it will cause problems for other tests running against this service. Instead, create a new tree that is independent and work with it. The current implementation of tests causes side effects that lead to instability in all tests.

@ssulka ssulka requested a review from osmman March 18, 2025 09:32
&cli{
Name: "createtree",
setupStrategy: PreferredSetupStrategy(),
versionCommand: "version",
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no version command in createtree cli

Copy link
Contributor

@osmman osmman left a comment

Choose a reason for hiding this comment

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

I must request changes based on the following concerns: the test suite still depends heavily on an OpenShift (OCP) deployment and modifies the RHTAS instance, which is not aligned with the intended scope of this repository. These deployment-specific tests should reside in projects such as securesign/secure-sign-operator or the related Ansible project. Additionally, the original Jira issue specified a simple smoke test aimed solely at verifying the CLI binary's functionality (for example, by using the help command), so the current approach exceeds that scope. As next steps, you can move your current work into the operator repository and extend it to cover the full Rekor key rotation scenario—especially since we already have a scenario for Fulcio, so your achieved work won't go to waste. For this repository, please modify the PR to simply execute the createtree and updatetree CLI binaries by running the help command and checking that they have executed successfully.

&cli{
Name: "updatetree",
setupStrategy: PreferredSetupStrategy(),
versionCommand: "version",
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no version command in updatetree cli. It looks like correct values should be empty string.

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.

2 participants