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

Validate Multi-Namespace support #64

Merged
merged 1 commit into from
Apr 7, 2021

Conversation

nitishkumar71
Copy link
Member

@nitishkumar71 nitishkumar71 commented Apr 6, 2021

Signed-off-by: Nitishkumar Singh [email protected]

Closes #63

Testing Steps

  1. Create Kind Cluster
    kind create cluster --name fass-pv-test
  2. install openfaas using arkade
    arkade install openfaas --basic-auth=false --clusterrole
  3. Port Forward gateway to 8080
    kubectl port-forward -n openfaas svc/gateway 8080:8080 &
  4. Annotate kubernetes namespace
kubectl create namespace staging-fn
kubectl annotate namespace/staging-fn openfaas="1"
kubectl create namespace dev
kubectl annotate namespace/dev openfaas="1"
  1. Export CERTIFIER_NAMESPACES
    export CERTIFIER_NAMESPACES=staging-fn,dev
  2. Test K8 provider
    make test-kubernetes

@nitishkumar71 nitishkumar71 changed the title [WIP] Validate Multi-Namespace support Validate Multi-Namespace support Apr 6, 2021
@nitishkumar71
Copy link
Member Author

@LucasRoesler Please review it.

Copy link
Member

@LucasRoesler LucasRoesler left a comment

Choose a reason for hiding this comment

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

@nitishkumar71 there are a few small language changes that I think make the errors a little more consistent with the other tests or improve the readability slightly.

There is also one slightly larger request to allow the default namespace to be configurable as well.

But it otherwise looks great 👍

@@ -121,6 +123,30 @@ func Test_Deploy_WithAnnotations(t *testing.T) {
}
}

func Test_DeploymentNamespaces(t *testing.T) {
defaultNamespace := "openfaas-fn"
Copy link
Member

Choose a reason for hiding this comment

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

I think we should move this to an ENV config as well: CERTIFIER_DEFAULT_NAMESPACE we can set this to openfaas-fn if it is empty This will allow us to run it against non-standard installations as well, so that we have

expectedNamespaces := append(config.Namespaces, config.DefaultNamespace)

tests/deploy_test.go Outdated Show resolved Hide resolved
tests/deploy_test.go Outdated Show resolved Hide resolved
tests/deploy_test.go Outdated Show resolved Hide resolved
tests/deploy_test.go Outdated Show resolved Hide resolved
@derek derek bot added the no-dco label Apr 7, 2021
@derek
Copy link

derek bot commented Apr 7, 2021

Thank you for your contribution. I've just checked and your commit doesn't appear to be signed-off. That's something we need before your Pull Request can be merged. Please see our contributing guide.
Tip: if you only have one commit so far then run: git commit --amend --signoff and then git push --force.

Signed-off-by: Nitishkumar Singh <[email protected]>

Apply suggestions from code review

Co-authored-by: Lucas Roesler <[email protected]>

Apply Reviews

Signed-off-by: Nitishkumar Singh <[email protected]>
@derek derek bot removed the no-dco label Apr 7, 2021
@nitishkumar71
Copy link
Member Author

@LucasRoesler Thank you for the review. I have made all the required changes.

Copy link
Member

@LucasRoesler LucasRoesler left a comment

Choose a reason for hiding this comment

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

Looks and works great 🚀

@LucasRoesler LucasRoesler merged commit bad219a into openfaas:master Apr 7, 2021
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.

Test the namespaces listing endpoint
2 participants