feat(Context based Restrictions): enforcement mode support #191

zhenwan · 2022-06-07T19:44:12Z

PR summary

Context based Restrictions API update to include enforcement mode support

PR Checklist

Please make sure that your PR fulfills the following requirements:

The commit message follows the Angular Commit Message Guidelines.
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

Current vs new behavior

add enforcement mode to rule

Does this PR introduce a breaking change?

Yes
[ x] No

Other information

CLAassistant · 2022-06-07T19:44:16Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

soohoowoohoo

I think most of the testing changes that were removed can be put back. I left comments on some of the high level samples that need to be duplicated everywhere. I guess the top items for me are:

Why are there 3 sets of enforcement mode constants. Can we just have 1 set?
Why was the NewServiceRefValue method removed. I didn't see a replacement for this.

soohoowoohoo · 2022-06-07T20:47:00Z

contextbasedrestrictionsv1/context_based_restrictions_v1.go

-// Options : Service options
-type Options struct {
+// ContextBasedRestrictionsV1Options : Service options
+type ContextBasedRestrictionsV1Options struct {


@zhenwan I think you can change this back to Options. I think this value is what was originally there and that Alberto might have updated it to just be plain Options

changed back

It's preferable to just leave the generated code as-is, otherwise you'll need to make this change whenever you regenerate the service and your service will be will be inconsistent with other services in the SDK.

@padamstx I agree with this statement. However, it seems the first person to publish already started making changes to the auto-generated names. My concern is that if we allow this longer name to stay that we'll have to do a major version bump since this would be a breaking change to the struct name. If you're ok with this (or have some other strategy to handle this), I'm ok with sticking with the auto-generated name.

Yes, it would be a breaking change to revert back to the auto-generated name, but the platform-services SDKs are technically still pre-release, so breaking changes do not need a major version bump.

soohoowoohoo · 2022-06-07T20:48:13Z

contextbasedrestrictionsv1/context_based_restrictions_v1.go

+const (
+	CreateRuleOptionsEnforcementModeDisabledConst = "disabled"
+	CreateRuleOptionsEnforcementModeEnabledConst  = "enabled"
+	CreateRuleOptionsEnforcementModeReportConst   = "report"
+)


@zhenwan I assume these are auto generated?

soohoowoohoo · 2022-06-07T20:51:15Z

contextbasedrestrictionsv1/context_based_restrictions_v1.go

+const (
+	ReplaceRuleOptionsEnforcementModeDisabledConst = "disabled"
+	ReplaceRuleOptionsEnforcementModeEnabledConst  = "enabled"
+	ReplaceRuleOptionsEnforcementModeReportConst   = "report"
+)


Another enforcement mode reference if changes need to be made

those are auto-generated, I will delete them

These are generated as a result of the fact that the corresponding schema defines an enum. Why delete the const definitions here???

@padamstx if everything is populated from the swagger it seems that we should check our swagger to make sure we have 1 definition that gets reused then. @zhenwan can you verify whether we have 1 or multiple definitions?

The duplicates might be generated due to the way in which the options model structs are synthesized. The generator will "explode" body vars which means that when representing a request body within the options model struct, the generator will essentially copy each of the request body's top-level properties (assume the request body is defined as an "object"). This might result in duplicates because the options model struct AND the original request body schema would now both define the "enforcement_mode" property which (I assume) contains the enum definition.
Viewing the duplicates through that lens, it makes sense that we're seeing the duplicate definitions. My suggestion would be to just leave the duplicate const definitions there since any changes to the generated code will need to be applied every time you re-gen the service.

soohoowoohoo · 2022-06-07T20:52:19Z

contextbasedrestrictionsv1/context_based_restrictions_v1.go

+const (
+	RuleEnforcementModeDisabledConst = "disabled"
+	RuleEnforcementModeEnabledConst  = "enabled"
+	RuleEnforcementModeReportConst   = "report"
+)


Why do we have a third set of constants?

those are auto-generated , I will delete them

soohoowoohoo · 2022-06-07T20:53:08Z

contextbasedrestrictionsv1/context_based_restrictions_v1.go

-func (*ContextBasedRestrictionsV1) NewServiceRefValue(accountID string) (_model *ServiceRefValue, err error) {
-	_model = &ServiceRefValue{
-		AccountID: core.StringPtr(accountID),
-	}
-	err = core.ValidateStruct(_model, "required parameters")
-	return
-}


What happened to this function?

The code generator deleted it. I will added it back according to your comments below

We should first understand what changed in the API definition that caused this to no longer be generated. We can't just add and delete stuff from the generated service code on a whim.

soohoowoohoo · 2022-06-07T21:02:12Z

contextbasedrestrictionsv1/context_based_restrictions_v1_test.go

-			It(`Invoke NewServiceRefValue successfully`, func() {
-				accountID := "testString"
-				_model, err := contextBasedRestrictionsService.NewServiceRefValue(accountID)
-				Expect(_model).ToNot(BeNil())
-				Expect(err).To(BeNil())
-			})


I think should be kept.

soohoowoohoo · 2022-06-07T21:03:12Z

contextbasedrestrictionsv1/context_based_restrictions_v1_examples_test.go


-var _ = Describe(`contextbasedrestrictionsv1.ContextBasedRestrictionsV1 Examples Tests`, func() {


The package prefix can stay.

The package prefixes are removed by the new code generator.

soohoowoohoo · 2022-06-07T21:03:32Z

contextbasedrestrictionsv1/context_based_restrictions_v1_examples_test.go

@@ -63,22 +62,23 @@ var _ = Describe(`contextbasedrestrictionsv1.ContextBasedRestrictionsV1 Examples

 	var shouldSkipTest = func() {
 		if !configLoaded {
-			Skip("External configuration is not available, skipping tests...")


we can probably keep calling these tests instead of examples.

yes, if external configuration file is missing, example and integration tests will be skipped, but not the unit tests.

This change is due to the new version of the generator.

soohoowoohoo · 2022-06-07T21:04:12Z

contextbasedrestrictionsv1/context_based_restrictions_v1_examples_test.go

@@ -132,9 +136,9 @@ var _ = Describe(`contextbasedrestrictionsv1.ContextBasedRestrictionsV1 Examples
 			}

 			createZoneOptions := contextBasedRestrictionsService.NewCreateZoneOptions()
-			createZoneOptions.SetName("SDK TEST - an example of zone")


We probably want to keep the SDK TEST - prefix for our own testing.

I removed them to use the default one so next time when we run the code generator we will have less thing to update

These values come from the example fields defined in the API definition.

soohoowoohoo

Looks good from my end.

padamstx

LGTM

@zhenwan Let me know when you're ready for me to merge this PR. I assume you'll be making similar changes to the other 3 SDKs, is that right?

zhenwan · 2022-06-10T15:22:49Z

@padamstx Please merge this PR. thank you!
Yes, we will work on other 3 SDKs too.

# [0.26.0](v0.25.2...v0.26.0) (2022-06-23) ### Features * **Context based Restrictions:** enforcement mode support ([#191](#191)) ([f049e95](f049e95))

ibm-devx-sdk · 2022-06-23T17:18:13Z

🎉 This PR is included in version 0.26.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

enforcement mode support

ce02086

soohoowoohoo reviewed Jun 7, 2022

View reviewed changes

zhenwan added 2 commits June 7, 2022 19:16

updated on Stephen's comments

392bd4f

updated based on last reviews

b4528db

soohoowoohoo approved these changes Jun 10, 2022

View reviewed changes

padamstx approved these changes Jun 10, 2022

View reviewed changes

examples updates in swagger.yaml

c420c90

zhenwan mentioned this pull request Jun 18, 2022

CBR: context-based-restriction update for Enforcement mode support IBM-Cloud/terraform-provider-ibm#3853

Merged

dpopp07 merged commit f049e95 into IBM:main Jun 23, 2022

ibm-devx-sdk pushed a commit that referenced this pull request Jun 23, 2022

chore(release): 0.26.0 release notes [skip ci]

74ecd70

# [0.26.0](v0.25.2...v0.26.0) (2022-06-23) ### Features * **Context based Restrictions:** enforcement mode support ([#191](#191)) ([f049e95](f049e95))

ibm-devx-sdk added the released label Jun 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Context based Restrictions): enforcement mode support #191

feat(Context based Restrictions): enforcement mode support #191

zhenwan commented Jun 7, 2022

CLAassistant commented Jun 7, 2022

soohoowoohoo left a comment

soohoowoohoo Jun 7, 2022

zhenwan Jun 7, 2022

padamstx Jun 8, 2022

soohoowoohoo Jun 8, 2022

padamstx Jun 8, 2022

soohoowoohoo Jun 7, 2022

zhenwan Jun 7, 2022

soohoowoohoo Jun 7, 2022

zhenwan Jun 7, 2022 •

edited

Loading

padamstx Jun 8, 2022

soohoowoohoo Jun 8, 2022 •

edited

Loading

padamstx Jun 8, 2022

soohoowoohoo Jun 7, 2022

zhenwan Jun 7, 2022 •

edited

Loading

soohoowoohoo Jun 7, 2022

zhenwan Jun 7, 2022

padamstx Jun 8, 2022

soohoowoohoo Jun 7, 2022

zhenwan Jun 7, 2022

soohoowoohoo Jun 7, 2022

zhenwan Jun 8, 2022

soohoowoohoo Jun 7, 2022

zhenwan Jun 8, 2022

padamstx Jun 8, 2022

soohoowoohoo Jun 7, 2022

zhenwan Jun 7, 2022

padamstx Jun 8, 2022

soohoowoohoo left a comment

padamstx left a comment •

edited

Loading

zhenwan commented Jun 10, 2022

ibm-devx-sdk commented Jun 23, 2022


		var _ = Describe(`contextbasedrestrictionsv1.ContextBasedRestrictionsV1 Examples Tests`, func() {

feat(Context based Restrictions): enforcement mode support #191

feat(Context based Restrictions): enforcement mode support #191

Conversation

zhenwan commented Jun 7, 2022

PR summary

PR Checklist

Current vs new behavior

Does this PR introduce a breaking change?

Other information

CLAassistant commented Jun 7, 2022

soohoowoohoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhenwan Jun 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soohoowoohoo Jun 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhenwan Jun 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soohoowoohoo left a comment

Choose a reason for hiding this comment

padamstx left a comment • edited Loading

Choose a reason for hiding this comment

zhenwan commented Jun 10, 2022

ibm-devx-sdk commented Jun 23, 2022

zhenwan Jun 7, 2022 •

edited

Loading

soohoowoohoo Jun 8, 2022 •

edited

Loading

zhenwan Jun 7, 2022 •

edited

Loading

padamstx left a comment •

edited

Loading