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.

Sign up for GitHub

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

Jump to bottom

test: lint and add tests for helmfile move #853

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

test: lint and add tests for helmfile move #853

wants to merge 1 commit into from

Conversation

ankitm123
Copy link
Contributor

Signed-off-by: ankitm123 [email protected]

@ankitm123
Copy link
Contributor Author

/cc @msvticket @tomhobson @babadofar

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@jenkins-x-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign ankitm123
You can assign the PR to them by writing /assign @ankitm123 in a comment when ready.

The full list of commands accepted by this bot can be found 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

ankitm123
ankitm123 commented Jun 17, 2022
View reviewed changes
pkg/cmd/helmfile/move/move.go
o.ClusterWide = make(map[string]bool)
JXClient, err := jxclient.LazyCreateJXClient(nil)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we need jxclient? Unit tests pass with normal kubeclient, but I am not sure ...

ankitm123
ankitm123 commented Jun 17, 2022
View reviewed changes
pkg/cmd/helmfile/move/move.go
@@ -330,21 +335,19 @@ func (o *Options) moveFilesToClusterOrNamespacesFolder(dir, ns, releaseName, cha
return nil
}

func (o *Options) isClusterWide(kind string, apiVersion string, client versioned.Interface) (bool, error) {
if kube.IsNoKubernetes() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we need this anymore?

@msvticket
Copy link
Contributor

It complicates things unnecessarily that you opened a PR for the same part of the code as #850. I think you should close this and then we can cooperate on #850.

@msvticket
Copy link
Contributor

Also I don't what is so bad in needing JX_NO_KUBERNETES=true, where there is already KUBECONFIG=/cluster/connections/not/allowed.

@ankitm123
Copy link
Contributor Author

It complicates things unnecessarily that you opened a PR for the same part of the code as #850

There were some linting issues as well, which were seen in another PR, I am ok closing this one (let's just copy over the tests and lint fixes)

Also I don't what is so bad in needing JX_NO_KUBERNETES=true,

I dont see the point in using this variable for unit tests. With JX_NO_KUBERNETES, we are not testing parts of the code which are easily tested using the kubernetes fake client (as demonstrated in this PR). Also there have been discussions around moving away from github actions, so this function should be eventually removed imo cc @tomhobson
(apart from codeql and may be codecov, dont see a point in using github actions - testing jx with jx is the right way to go - the v2 way 😬)

@tomhobson
Copy link
Contributor

I dont see the point in using this variable for unit tests. With JX_NO_KUBERNETES, we are not testing parts of the code which are easily tested using the kubernetes fake client (as demonstrated in this PR). Also there have been discussions around moving away from github actions, so this function should be eventually removed imo cc @tomhobson

I think from our point of view, we should be using our own complete pipelines. But if someone wants to use jx version in their github action then I don't see why we should prevent them from doing that.

It's nice to have the optionality, but it also greatly improves complexity.

@ankitm123
Copy link
Contributor Author

But if someone wants to use jx version in their github action then I don't see why we should prevent them from doing that.

In that case, we should have 2 test cases - one with JX_NO_KUBERNETES and one where we test the kubernetes part (using the fake client)

@msvticket
Copy link
Contributor

I dont see the point in using this variable for unit tests. With JX_NO_KUBERNETES, we are not testing parts of the code which are easily tested using the kubernetes fake client (as demonstrated in this PR).

Actually this PR demonstrates that it is not easy to test, since the test passes while in reality the code failed in the bdd tests.

@ankitm123
Copy link
Contributor Author

ankitm123 commented Jun 20, 2022 •
edited
Loading

Actually this PR demonstrates that it is not easy to test, since the test passes while in reality the code failed in the bdd tests.

That is because we only have one test case - only test the happy path (hence in draft mode) - I would like to replicate the bdd test failure using unit tests tbh (I will search the logs).

Other reasons could be the test data is not sufficient. I firmly we believe our testing (unit and integration) will improve with time :)

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

@babadofar babadofar Awaiting requested review from babadofar

@msvticket msvticket Awaiting requested review from msvticket

@tomhobson tomhobson Awaiting requested review from tomhobson

Assignees
No one assigned
Labels
do-not-merge/work-in-progress size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ankitm123 @jenkins-x-bot @msvticket @tomhobson