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

Dummy e2e Test #29

Merged
merged 3 commits into from
Mar 27, 2023
Merged

Dummy e2e Test #29

merged 3 commits into from
Mar 27, 2023

Conversation

razo7
Copy link
Member

@razo7 razo7 commented Mar 23, 2023

Basic and dummy E2E test which only creates CR, verify it was created and then deletes it. The FAR CR is not running any FA, and only echo an empty string -> echo " "
This way we can catch installation/running errors, such as #26, because deployment would fail until we add functionality of E2E test in #20.

Using Ginkgo commands to bootsrap a new suite - e2e
Test that the dummy CR has been created
@openshift-ci openshift-ci bot requested review from clobrano and slintes March 23, 2023 11:56
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Mar 23, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: razo7

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:

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

"github.com/medik8s/fence-agents-remediation/api/v1alpha1"
)

const fenceAgentDummy = "echo"
Copy link
Contributor

Choose a reason for hiding this comment

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

(very) nit: I think the name of this variable could be more descriptive. Is it the Agent name?

Copy link
Member

Choose a reason for hiding this comment

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

Nit: IMO var value could be better (i.e for example dummy-fence-agent)

Copy link
Member Author

@razo7 razo7 Mar 27, 2023

Choose a reason for hiding this comment

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

echo is just an example to dummy-fence-agent and I doubt it would run without a failure. In the end a pod will try to run a cli command dummy-fence-agent, and I have used echo since it is very basic Unix-like operating system command.

})
})

// createFAR assigns the input to FenceAgentsRemediation object, creates CR it with offset, and returns the CR object
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a maybe a typo here? "creates CR it with offset"

Copy link
Member

Choose a reason for hiding this comment

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

Nit: IIUC the offset refers to the error reporting and not the CR creation

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct I will fix it in a follow up

})

// createFAR assigns the input to FenceAgentsRemediation object, creates CR it with offset, and returns the CR object
func createFAR(nodeName string, agent string, sharedparameters map[v1alpha1.ParameterName]string, nodeparameters map[v1alpha1.ParameterName]map[v1alpha1.NodeName]string) *v1alpha1.FenceAgentsRemediation {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use camel case naming format might be helpful
nodeParameters, sharedParameters

Better variable names and typos in comments
@mshitrit
Copy link
Member

/lgtm
/hold
left 2 nits, feel free to fix/unhold

@razo7
Copy link
Member Author

razo7 commented Mar 27, 2023

/unhold

@openshift-merge-robot openshift-merge-robot merged commit a8fdbb6 into medik8s:main Mar 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants