8000 THREESCALE-8128 remove unnecesary camel case removal by MStokluska · Pull Request #865 · 3scale/3scale-operator · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

THREESCALE-8128 remove unnecesary camel case removal #865

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 13, 2023
Merged

THREESCALE-8128 remove unnecesary camel case removal #865

merged 1 commit into from
Sep 13, 2023

Conversation

MStokluska
Copy link
Contributor
@MStokluska MStokluska commented Sep 11, 2023 •
edited
Loading

Jira: https://issues.redhat.com/browse/THREESCALE-8128

What

Removed the toLower when making validation of backend and product spec. Few years ago 3scale api did not support the camelCase system name, it does now.

Verify

From this branch:

  • run to create new project
oc new-project threescale
  • run to install CRDs
make install
  • create dummy secret
oc apply -f - <<EOF   
---
apiVersion: v1
kind: Secret
metadata:
  creationTimestamp: null
  name: s3-credentials
stringData:
  AWS_ACCESS_KEY_ID: something
  AWS_SECRET_ACCESS_KEY: something
  AWS_BUCKET: something
  AWS_REGION: us-east-1
type: Opaque
EOF
  • fetch cluster domain
DOMAIN=$(oc get routes console -n openshift-console -o json | jq -r '.status.ingress[0].routerCanonicalHostname' | sed 's/router-default.//')
  • create apim
kubectl apply -f - <<EOF                                                        
---
apiVersion: apps.3scale.net/v1alpha1
kind: APIManager
metadata:
  name: apimanager-sample
  namespace: threescale
spec:
  system:
    fileStorage:
      simpleStorageService:
        configurationSecretRef:
          name: s3-credentials
  wildcardDomain: $DOMAIN
EOF
  • create OpenAPI yaml secret
kubectl apply -f - <<EOF               
---
kind: Secret
apiVersion: v1
metadata:
  name: myopenapi
stringData:
 myopenapi.yaml: |
  ---
  openapi: "3.0.2"
  info:
    title: "camelCaseSystem"
    description: "some description"
    version: "1.0.0"
  servers:
   - url: https://echo-api.3scale.net:443
  paths:
    /:
      get:
        operationId: "get"
        responses:
          405:
            description: "invalid input"
type: Opaque
EOF

Creating OpenAPI with productSystemName specified as camel case

  • create OpenAPI CR
kubectl apply -f - <<EOF                                                        
---
apiVersion: capabilities.3scale.net/v1beta1
kind: OpenAPI
metadata:
  name: camelcase
  namespace: threescale
spec:
  openapiRef:
    secretRef:
      name: myopenapi
      namespace: threescale
  productSystemName: camelCaseSys
EOF
  • confirm that status of OpenAPI CR is "Ready" and "True"
  • login to 3scale tenant as admin and confirm the backend and product are there and linked.

Remove the openapi CR and confirm all assets related to it are gone.

Not specifying the productSystemName but using camel case in the openapi secret

  • Create another OpenAPI CR:
kubectl apply -f - <<EOF                                                        
---
apiVersion: capabilities.3scale.net/v1beta1
kind: OpenAPI
metadata:
  name: camelcase
  namespace: threescale
spec:
  openapiRef:
    secretRef:
      name: myopenapi
      namespace: threescale
EOF

Verify that the OpenAPI CR was created successfully.

@MStokluska MStokluska requested a review from a team as a code owner September 11, 2023 14:25
@MStokluska
Copy link
Contributor Author

/test test-e2e infra error

@valerymo
Copy link
Contributor

Hi @MStokluska
By default, the system name is taken from the info.title field in the OpenAPI definition (if it is not present in openApiCR).
It's stated in the documentation and in the code, and there - the conversion to lowercase is produced.
https://github.com/3scale/3scale-operator/blob/master/doc/openapi-user-guide.md#product-name
https://github.com/3scale/3scale-operator/blob/master/pkg/helper/openapi_utils.go#L25-L28

Suppose it was the reason that in product_types the conversion was done in very initial versions.

Maybe we could try find another way for fix, as for example for product, maybe do change here (?) - https://github.com/3scale/3scale-operator/blob/master/controllers/capabilities/product_threescale_reconciler.go#L70-L78 . For backend seems it's differ.

Anyway, I tried to check PR in my cluster, according to validation notes, but I still see Status False

Spec:
  Openapi Ref:
    Secret Ref:
      Name:             myopenapi
      Namespace:        3scale-test
  Product System Name:  camelCaseSys
Status:
  Backend Resource Names:
    Name:  camelcasesystem-44d59b83-f842-45b1-bc42-5a5ec51569d8
  Conditions:
    Last Transition Time:  2023-09-12T07:29:58Z
    Status:                False
    Type:                  Failed
    Last Transition Time:  2023-09-12T07:29:58Z
    Status:                False
    Type:                  Invalid
    Last Transition Time:  2023-09-12T07:29:58Z
    Status:                False
    Type:                  Ready
  Observed Generation:     1
  Product Resource Name:
    Name:                 camelcasesystem-44d59b83-f842-45b1-bc42-5a5ec51569d8
  Provider Account Host:  https://3scale-admin.apps.vmogilev01.41x3.s1.devshift.org
Events:                   <none>

Sorry, seems to me, solution could be differ, do no remove Spec.SystemName = systemNameLowercase in product_types (and backend)
Thank you

valerymo
valerymo reviewed Sep 12, 2023
View reviewed changes
valerymo
valerymo reviewed Sep 12, 2023
View reviewed changes
@MStokluska
Copy link
Contributor Author

Hi @MStokluska By default, the system name is taken from the info.title field in the OpenAPI definition (if it is not present in openApiCR). It's stated in the documentation and in the code, and there - the conversion to lowercase is produced. https://github.com/3scale/3scale-operator/blob/master/doc/openapi-user-guide.md#product-name https://github.com/3scale/3scale-operator/blob/master/pkg/helper/openapi_utils.go#L25-L28

Suppose it was the reason that in product_types the conversion was done in very initial versions.

Maybe we could try find another way for fix, as for example for product, maybe do change here (?) - https://github.com/3scale/3scale-operator/blob/master/controllers/capabilities/product_threescale_reconciler.go#L70-L78 . For backend seems it's differ.

Anyway, I tried to check PR in my cluster, according to validation notes, but I still see Status False

Spec:
  Openapi Ref:
    Secret Ref:
      Name:             myopenapi
      Namespace:        3scale-test
  Product System Name:  camelCaseSys
Status:
  Backend Resource Names:
    Name:  camelcasesystem-44d59b83-f842-45b1-bc42-5a5ec51569d8
  Conditions:
    Last Transition Time:  2023-09-12T07:29:58Z
    Status:                False
    Type:                  Failed
    Last Transition Time:  2023-09-12T07:29:58Z
    Status:                False
    Type:                  Invalid
    Last Transition Time:  2023-09-12T07:29:58Z
    Status:                False
    Type:                  Ready
  Observed Generation:     1
  Product Resource Name:
    Name:                 camelcasesystem-44d59b83-f842-45b1-bc42-5a5ec51569d8
  Provider Account Host:  https://3scale-admin.apps.vmogilev01.41x3.s1.devshift.org
Events:                   <none>

Sorry, seems to me, solution could be differ, do no remove Spec.SystemName = systemNameLowercase in product_types (and backend) Thank you

Hey, thanks Valery.
As per the PR desc, the system names were brought to lower because 3scale API calls 3 years ago did not support came cases. This is no longer a case and we should move away from this idea IMO. However, I was missing the removal of the "toLower" when dealing with default system name.

As for the error and reason your CR isn't progressing it's unrelated to this PR and is caused by missing routes due to another issue.

@valerymo
Copy link
Contributor

Hi Michal, please look at following places, that could be related:
https://github.com/3scale/3scale-operator/blob/master/pkg/helper/openapi_utils.go#L32
https://github.com/3scale/3scale-operator/blob/master/apis/capabilities/v1beta1/activedoc_types.go#L194-L203

@MStokluska
Copy link
Contributor Author
MStokluska commented Sep 13, 2023 •
edited by valerymo
Loading

Hi Michal, please look at following places, that could be related: https://github.com/3scale/3scale-operator/blob/master/pkg/helper/openapi_utils.go#L32 https://github.com/3scale/3scale-operator/blob/master/apis/capabilities/v1beta1/activedoc_types.go#L194-L203

@valerymo

As for link # 1 - active docs are not supported via openAPI CR - we can change it in another Jira
As for link # 2 - k8s CRs names must not contain capital characters therefore this conversion makes sense to stay

@MStokluska , agree. K8S - ok. ActiveDoc - it's not related to this Jira. No need changed it, it's own CR and Ccontroller. It has it's own ProductSystemName where case is not changed, and it used in controller. So looks like all is good in your PR for me. Thanks very much for checking and updates. It looks good for me.

@codeclimate
Copy link
codeclimate bot commented Sep 13, 2023

Code Climate has analyzed commit 6ea57ff and detected 0 issues on this pull request.

View more on Code Climate.

@valerymo
Copy link
Contributor

/lgtm

@MStokluska MStokluska merged commit 73d07ac into 3scale:master Sep 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@valerymo valerymo valerymo left review comments

@austincunningham austincunningham austincunningham approved these changes

Assignees
No one assigned
Labels
approve lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@MStokluska @valerymo @austincunningham
0