Skip to content
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

Registry operator full app name support #1441

Closed
9 tasks done
Tracked by #1274
michael-valdron opened this issue Feb 1, 2024 · 10 comments · Fixed by devfile/registry-operator#86
Closed
9 tasks done
Tracked by #1274

Registry operator full app name support #1441

michael-valdron opened this issue Feb 1, 2024 · 10 comments · Fixed by devfile/registry-operator#86
Assignees
Labels
area/registry Devfile registry for stacks and infrastructure

Comments

@michael-valdron
Copy link
Member

michael-valdron commented Feb 1, 2024

Which area/kind this issue is related to?

/area registry

Issue Description

The registry operator currently only has an app name that is used across the operator specifications: https://github.com/devfile/registry-operator/blob/f55ab8e7c8061934547c7d55bd67d81d509a8752/pkg/registry/naming.go

The helm chart uses both an app name and a fully qualified app name which a combo of the app name and release name by default: https://github.com/devfile/registry-support/blob/f66264f22310c3c93bf0c77db5f4cd4b560aba5e/deploy/chart/devfile-registry/templates/_template.tpl#L20-L39

The registry operator should implement a fully qualified app name as well and use this field in the same places as the helm chart does.

Parent Epic: #1274

Acceptance Criteria

  • Create a getter function to replicate how the full app name is built under the helm chart
  • Use the getter function to set the fields under the deployment spec as they are under helm chart deployment.yaml
  • Use the getter function to set the fields under the configmap spec as they are under helm chart configmap.yaml
  • Use the getter function to set the fields under the service spec as they are under helm chart service.yaml
  • Use the getter function to set the fields under the volume spec as they are under helm chart pvc.yaml
  • Use the getter function to set the fields under the ingress spec as they are under helm chart ingress.yaml
  • Use the getter function to set the fields under the route spec as they are under helm chart route.yaml
  • Create test case for new getter function
  • Verify integration testing is passing
@openshift-ci openshift-ci bot added the area/registry Devfile registry for stacks and infrastructure label Feb 1, 2024
@Jdubrick Jdubrick self-assigned this Mar 19, 2024
@michael-valdron
Copy link
Member Author

michael-valdron commented Mar 28, 2024

Reviewing the criteria of this issue, there is going to be some adjustments to what needs to be done under this issue as well as a correction to what was done in #1439:

  1. There will be more changes than a getter function as there is already functions taking care of setting metadata fields and labels that would need refactoring: https://github.com/devfile/registry-operator/blob/7b6929036378569680dce0eca9d7f4d963ec5fcc/pkg/registry/util.go
  2. Removal of the overrideName field from Add helm chart override fields to DevfileRegistry registry operator CRD #1439 as this could be set via the CR name DevfileRegistry.Name, rather we can expand to use the CR name as the app name and full name as the extension of the CR name to be set to metadata.name
    Skipping part of this step, overrideName is still needed, this was confusion between .Chart.Name and .Release.Name, overrideName would be used to override the static app name rather than the CR name DevfileRegistry.Name

@michael-valdron
Copy link
Member Author

Need to refactor naming.go functions to use fully qualified app name instead of CR name. This ensures the controller logic does not break due to resource name changes.

@michael-valdron
Copy link
Member Author

Need to refactor naming.go functions to use fully qualified app name instead of CR name. This ensures the controller logic does not break due to resource name changes.

michael-valdron/devfile-registry-operator@2a903d2

@michael-valdron
Copy link
Member Author

Verify integration testing is passing

DNS is currently failing to resolve hostname, investigating why this is happening. Once fixed, the open PR should be ready for review.

@michael-valdron
Copy link
Member Author

Verify integration testing is passing

DNS is currently failing to resolve hostname, investigating why this is happening. Once fixed, the open PR should be ready for review.

Captured error in test log:

------------------------------
• [FAILED] [147.747 seconds]
[Create Devfile Registry resource] [It] Should deploy a devfile registry on to the cluster
/home/mvaldron/source/repos/michael-valdron/devfile-registry-operator/tests/integration/pkg/tests/devfileregistry_tests.go:35

  [FAILED] Unexpected error:
      <*url.Error | 0xc000a5f020>: 
      Get "http://devfileregistry-devfile-registry-registry-operator-system.apps-crc.testing": dial tcp: lookup devfileregistry-devfile-registry-registry-operator-system.apps-crc.testing on 10.11.5.19:53: no such host
      {
          Op: "Get",
          URL: "http://devfileregistry-devfile-registry-registry-operator-system.apps-crc.testing",
          Err: <*net.OpError | 0xc0004be370>{
              Op: "dial",
              Net: "tcp",
              Source: nil,
              Addr: nil,
              Err: <*net.DNSError | 0xc00064aec0>{
                  Err: "no such host",
                  Name: "devfileregistry-devfile-registry-registry-operator-system.apps-crc.testing",
                  Server: "10.11.5.19:53",
                  IsTimeout: false,
                  IsTemporary: false,
                  IsNotFound: true,
              },
          },
      }
  occurred
  In [It] at: /home/mvaldron/source/repos/michael-valdron/devfile-registry-operator/tests/integration/pkg/tests/devfileregistry_tests.go:58 @ 04/11/24 13:03:53.479
------------------------------
SSS

Summarizing 1 Failure:
  [FAIL] [Create Devfile Registry resource] [It] Should deploy a devfile registry on to the cluster
  /home/mvaldron/source/repos/michael-valdron/devfile-registry-operator/tests/integration/pkg/tests/devfileregistry_tests.go:58

Ran 1 of 4 Specs in 159.879 seconds
FAIL! -- 0 Passed | 1 Failed | 0 Pending | 3 Skipped
--- FAIL: TestDevfileRegistryController (159.88s)
FAIL
make: *** [Makefile:119: test-integration] Error 1

@michael-valdron
Copy link
Member Author

#1519 could block this issue.

@michael-valdron
Copy link
Member Author

#1519 could block this issue.

Potential blocker resolved 👍

@michael-valdron
Copy link
Member Author

Verify integration testing is passing

DNS is currently failing to resolve hostname, investigating why this is happening. Once fixed, the open PR should be ready for review.

Captured error in test log:

------------------------------
• [FAILED] [147.747 seconds]
[Create Devfile Registry resource] [It] Should deploy a devfile registry on to the cluster
/home/mvaldron/source/repos/michael-valdron/devfile-registry-operator/tests/integration/pkg/tests/devfileregistry_tests.go:35

  [FAILED] Unexpected error:
      <*url.Error | 0xc000a5f020>: 
      Get "http://devfileregistry-devfile-registry-registry-operator-system.apps-crc.testing": dial tcp: lookup devfileregistry-devfile-registry-registry-operator-system.apps-crc.testing on 10.11.5.19:53: no such host
      {
          Op: "Get",
          URL: "http://devfileregistry-devfile-registry-registry-operator-system.apps-crc.testing",
          Err: <*net.OpError | 0xc0004be370>{
              Op: "dial",
              Net: "tcp",
              Source: nil,
              Addr: nil,
              Err: <*net.DNSError | 0xc00064aec0>{
                  Err: "no such host",
                  Name: "devfileregistry-devfile-registry-registry-operator-system.apps-crc.testing",
                  Server: "10.11.5.19:53",
                  IsTimeout: false,
                  IsTemporary: false,
                  IsNotFound: true,
              },
          },
      }
  occurred
  In [It] at: /home/mvaldron/source/repos/michael-valdron/devfile-registry-operator/tests/integration/pkg/tests/devfileregistry_tests.go:58 @ 04/11/24 13:03:53.479
------------------------------
SSS

Summarizing 1 Failure:
  [FAIL] [Create Devfile Registry resource] [It] Should deploy a devfile registry on to the cluster
  /home/mvaldron/source/repos/michael-valdron/devfile-registry-operator/tests/integration/pkg/tests/devfileregistry_tests.go:58

Ran 1 of 4 Specs in 159.879 seconds
FAIL! -- 0 Passed | 1 Failed | 0 Pending | 3 Skipped
--- FAIL: TestDevfileRegistryController (159.88s)
FAIL
make: *** [Makefile:119: test-integration] Error 1

Errors currently experienced seem to be linked to a current controller error when creating the route where the resultant address still exceeds 63 characters limit even with name truncation:

2024-04-12T14:00:10Z ERROR controllers.DevfileRegistry Failed to create new {"Route": "Route.Namespace", "registry-operator-system": "Service.Name", "registry-operator-system.Name": "devfileregistry-headless-devfile-registry", "error": "Route.route.openshift.io \"devfileregistry-headless-devfile-registry\" is invalid: spec.host: Invalid value: \"devfileregistry-headless-devfile-registry-registry-operator-system.apps.ci-ln-77n9r7b-76ef8.aws-2.ci.openshift.org\": must be no more than 63 characters"}

Investigating a workaround that can still sync with the helm chart's current <app-name>.<namespace> format, may require an additional issue be opened to provide a long term fix for both.

@michael-valdron
Copy link
Member Author

Errors currently experienced seem to be linked to a current controller error when creating the route where the resultant address still exceeds 63 characters limit even with name truncation:

2024-04-12T14:00:10Z ERROR controllers.DevfileRegistry Failed to create new {"Route": "Route.Namespace", "registry-operator-system": "Service.Name", "registry-operator-system.Name": "devfileregistry-headless-devfile-registry", "error": "Route.route.openshift.io \"devfileregistry-headless-devfile-registry\" is invalid: spec.host: Invalid value: \"devfileregistry-headless-devfile-registry-registry-operator-system.apps.ci-ln-77n9r7b-76ef8.aws-2.ci.openshift.org\": must be no more than 63 characters"}

Investigating a workaround that can still sync with the helm chart's current <app-name>.<namespace> format, may require an additional issue be opened to provide a long term fix for both.

Found a mistake in the ordering of the contains case for creating the fully qualified app name, was following the helm chart contains function parameter ordering which is backwards to the Go strings.Contains function. This helps workaround this problem at the time: devfile/registry-operator@3754509

Additionally, had to change the CR names used by the integration test cases to take advance of the case above, e.g. devfileregistry-tls becomes devfile-registry-tls: devfile/registry-operator@bba0d60

Will need to open an issue to investigate a proper fix for both the helm chart and registry operator to better reduce the chances of hitting the 63 character limit for OpenShift route addresses.

@michael-valdron
Copy link
Member Author

PR is ready for review: devfile/registry-operator#86

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/registry Devfile registry for stacks and infrastructure
Projects
Status: Done ✅
Development

Successfully merging a pull request may close this issue.

2 participants