-
Notifications
You must be signed in to change notification settings - Fork 101
Threescale 7135 - Product Promotion #742
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
|
Hi @Patryk-Stefanski. Thanks for your PR. I'm waiting for a 3scale member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@eguzki @MStokluska @KevFan |
|
One tip after reading the verification steps and before going to the code. You do not need to create a secret with the admin access token to use it as accountref. When you deploy 3scale, a default tenant is created ready to be used. And admin access token is already available in 3scale namespace's secret. The operator will read that for you if your CR is deployed in the same namespace as 3scale. So, your backend, product and ProxyConfigPromote resources do not need |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution! 🥳
I haven't gone through the details of the implementation. But let's discuss the current comments and I will review more later
| // Important: Run "make" to regenerate code after modifying this file | ||
|
|
||
| // product CR metadata.name | ||
| ProductCRName string `json:"productCRName,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a required field. Remove omitempty json tag
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
|
|
||
| // ProviderAccountRef references account provider credentials | ||
| // +optional | ||
| ProviderAccountRef *corev1.LocalObjectReference `json:"providerAccountRef,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think it is necessary. The provider account ref will be read from the product CR. Adding provider account ref opens the door to have product from tenant A and proxyconfigpromote from tenant B having reference to the previous product.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes good idea, we can make this change.
|
|
||
| // Environment you wish to promote to, if not present defaults to staging and if set to true promotes to production | ||
| // +optional | ||
| Production bool `json:"production,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if it's optional, better to be a pointer to bool. That way you can differentiate between when the user did not set and when the user set that to false. Maybe not relevant for this use case, but still to be consistent with the other optional attributes in the operator CRDs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
|
|
||
| // deleteCR deletes this CR when it has successfully completed the promotion | ||
| // +optional | ||
| DeleteCR bool `json:"deleteCR,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!
As commented above, better to be a pointer to bool.
you can define a handy ProxyConfigPromote struct method to abstract the pointer type
func (p *ProxyConfigPromote) IsDeleteCR() bool {
if p.Spec.DeleteCR == nil {
return false
}
return *p.Spec.DeleteCR
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
117e646 to
7e3dc70
Compare
7e3dc70 to
e46f568
Compare
7d88cf6 to
1a11ca5
Compare
|
@MStokluska & @KevFan if you have time for a review |
4be6c3f to
2016d2d
Compare
4d7db25 to
8493c13
Compare
c665b93 to
20bf2c1
Compare
402e753 to
39f18b8
Compare
|
@eguzki thinking the unit-test failing on circle-ci is some issue on the circle-ci side as rerunning the test will fail for a different reason each time its run with no code changes. The same unit test pass on the |
|
The weird thing is that in master branch, tests pass. |
fa51423 to
0b84cdc
Compare
0b84cdc to
d92f15d
Compare
|
Code Climate has analyzed commit d92f15d and detected 18 issues on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
|
@eguzki yep looks like memory footprint resource_class: largeto the unit tests and now passing on circleci |

Threescale-7135
What
Addition of proxyconfigpromote CRD that allows for single-use product promotion to stage or production.
Verification
2.Create dummy smtp secret
3.Create apimanager resource
cluster domain example :
apps.pstefans.uq32.s1.devshift.orgIn 3scale UI check that product1 configuration was promoted to staging
Create proxyconfigpromote resource to promote a product to production and remove resource after successful promotion