Skip to content

Add namespaced ui-api-acceptance tests #1132

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

Conversation

magicmatatjahu
Copy link
Contributor

@magicmatatjahu magicmatatjahu commented Oct 5, 2018

Description

Changes proposed in this pull request:

  • Add namespaced ui-api-acceptance tests for ClusterServiceBroker, ServiceBroker, ClusterServiceClass, ServiceClass,
  • Add serviceBrokers to ClusterRole

Related issue(s)
See also #952

@magicmatatjahu magicmatatjahu added kind/feature Categorizes issue or PR as related to a new feature. WIP area/service-management Issues or PRs related to service management area/busola Related to all activities around the Busola UI and all its views labels Oct 5, 2018
@magicmatatjahu magicmatatjahu force-pushed the namespaced-resources-tests branch 5 times, most recently from 3404666 to c03ae14 Compare October 5, 2018 07:16
@magicmatatjahu magicmatatjahu removed the WIP label Oct 5, 2018
@magicmatatjahu magicmatatjahu force-pushed the namespaced-resources-tests branch 2 times, most recently from 11b4164 to 24a6194 Compare October 5, 2018 12:15
Copy link
Contributor

@pkosiec pkosiec left a comment

Choose a reason for hiding this comment

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

Please use everywhere full names for resources, e.g.: clusterServiceClass, serviceClass,
the same thing with brokers

@@ -40,11 +40,11 @@ type ClusterServiceClass struct {
content map[string]interface{}
}

type classesQueryResponse struct {
type clusterClassesQueryResponse struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd use full name: clusterServiceClass & serviceClass everywhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


log.Printf("Broker %s still not ready. Waiting...\n", brokerName)
arr := broker.Status.Conditions
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this fragment can be extracted:

	for _, v := range arr {
				if v.Type == "Ready" {
					return v.Status == "True", nil
				}
			}
			log.Printf("%s %s still not ready. Waiting...\n", typeOfBroker, brokerName)

Because in brokers there is the same CommonServiceBrokerStatus type.

Copy link
Contributor Author

@magicmatatjahu magicmatatjahu Oct 6, 2018

Choose a reason for hiding this comment

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

Done. I changed this function to:

func checkStatusOfBroker(conditions []v1beta1.ServiceBrokerCondition) bool {
	for _, v := range conditions {
		if v.Type == "Ready" {
			return v.Status == "True"
		}
	}
	return false
}

func waitForBroker(brokerName, typeOfBroker string, svcatCli *clientset.Clientset) error {
	return waiter.WaitAtMost(func() (bool, error) {
		var conditions []v1beta1.ServiceBrokerCondition

		if typeOfBroker == tester.ClusterServiceBroker {
			broker, err := svcatCli.ServicecatalogV1beta1().ClusterServiceBrokers().Get(brokerName, metav1.GetOptions{})

			if err != nil || broker == nil {
				return false, err
			}

			conditions = broker.Status.Conditions
		} else {
			broker, err := svcatCli.ServicecatalogV1beta1().ServiceBrokers(tester.DefaultNamespace).Get(brokerName, metav1.GetOptions{})

			if err != nil || broker == nil {
				return false, err
			}

			conditions = broker.Status.Conditions
		}

		if checkStatusOfBroker(conditions) {
			return true, nil
		}

		log.Printf("%s %s still not ready. Waiting...\n", typeOfBroker, brokerName)
		return false, nil
	}, brokerReadyTimeout)
}

@magicmatatjahu magicmatatjahu force-pushed the namespaced-resources-tests branch from 24a6194 to 8ed2fd5 Compare October 6, 2018 08:44
@magicmatatjahu magicmatatjahu force-pushed the namespaced-resources-tests branch 3 times, most recently from 21798cf to e68ae49 Compare October 7, 2018 10:17
Copy link
Contributor

@pkosiec pkosiec left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@pkosiec pkosiec left a comment

Choose a reason for hiding this comment

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

Please edit ClusterRole to include servicebroker privilege.

2018/10/08 07:40:44 servicebrokers.servicecatalog.k8s.io "ups-broker" is forbidden: User "system:serviceaccount:kyma-system:test-core-core-ui-api-acceptance" cannot get servicebrokers.servicecatalog.k8s.io in the namespace "ui-api-acceptance"

@magicmatatjahu magicmatatjahu force-pushed the namespaced-resources-tests branch from 8911ff7 to c3d6860 Compare October 8, 2018 09:17
Copy link
Contributor

@hudymi hudymi left a comment

Choose a reason for hiding this comment

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

LGTM

require.NoError(t, err)

expectedResource := broker()
resourceDetailsQuery := `
Copy link
Contributor

Choose a reason for hiding this comment

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

Fix indents.

@magicmatatjahu magicmatatjahu force-pushed the namespaced-resources-tests branch from c3d6860 to d83ae76 Compare October 8, 2018 10:41
@magicmatatjahu magicmatatjahu force-pushed the namespaced-resources-tests branch from d83ae76 to 2c38b5c Compare October 8, 2018 10:44
@magicmatatjahu magicmatatjahu merged commit 43d564e into kyma-project:master Oct 8, 2018
@magicmatatjahu magicmatatjahu deleted the namespaced-resources-tests branch October 8, 2018 14:08
grischperl pushed a commit to grischperl/kyma that referenced this pull request Nov 10, 2020
* exit script, if version is not set, do not continue installer
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/busola Related to all activities around the Busola UI and all its views area/service-management Issues or PRs related to service management kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants