Skip to content

Add support for defining labels in Helm Broker bundles #1444

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

Merged
merged 1 commit into from
Oct 30, 2018

Conversation

mszostok
Copy link
Contributor

@mszostok mszostok commented Oct 26, 2018

Description

Changes proposed in this pull request:

  • Add support for defining labels in Helm Broker bundles
  • Fix behaviour of schema population in catalog.go: if schema not provided in bundle then the empty schema is not exported to Service Catalog but the object is still nil.

Related issue(s)

@mszostok mszostok added kind/feature Categorizes issue or PR as related to a new feature. area/service-management Issues or PRs related to service management labels Oct 26, 2018
@mszostok mszostok added this to the Sprint_Gopher_3 milestone Oct 26, 2018
Copy link
Contributor Author

@mszostok mszostok left a comment

Choose a reason for hiding this comment

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

FYI

metaCopy.Labels = map[string]string{}
}
// Business requirement that helm bundles are always treated as local
metaCopy.Labels["local"] = "true"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI: hardcoded here, can be replaced with

for k, v := range overridesForPlanLabels {
	metaCopy.Labels[k]  = v
}

where globaly we have

var overridesForPlanLabels = map[string]string{
        // Business requirement that helm bundles are always treated as local
	"local": "true",
}

but IMO for now it's not necessary.

@@ -69,60 +76,40 @@ func TestGetCatalogOnConversionError(t *testing.T) {
func TestBundleConversion(t *testing.T) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactor inspired by golden testing, see: https://golang.org/src/go/doc/doc_test.go#L119

updateGoldenFileIfRequested(t, goldenPath, normalizedGotSvc)

exp := tc.fixtureMarshaledOsbService(t, goldenPath)
assert.JSONEq(t, exp, string(normalizedGotSvc))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to assert on Go obj
using sth like that:

func fixtureOsbService(t *testing.T, testdataBasePath string) osb.Service {
	data, err := ioutil.ReadFile(testdataBasePath)
	require.NoError(t, err, "failed reading .golden")

	out := osb.Service{}
	require.NoError(t, json.Unmarshal(data, &out))
	return out
}

Unfortunately kinds are different, e.g. map[string]string vs Labels
where Labels is type Labels map[string]string. Then in the assertion, it cannot be compared easily. Because of that, I decided to normalize it to the JSON form.

@PK85 PK85 modified the milestones: Sprint_Gopher_3, Sprint_Gopher_4 Oct 29, 2018
Copy link
Member

@piotrmiskiewicz piotrmiskiewicz left a comment

Choose a reason for hiding this comment

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

lgtm

@mszostok mszostok merged commit 41d20df into kyma-project:master Oct 30, 2018
@mszostok mszostok deleted the hb-bundles-labels branch October 30, 2018 09:24
Maladie pushed a commit to Maladie/kyma that referenced this pull request Nov 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

3 participants