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.

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

Feature: Fully Qualified App Name #86

Merged
merged 10 commits into from
Apr 15, 2024
Merged

Feature: Fully Qualified App Name #86

merged 10 commits into from
Apr 15, 2024

Conversation

michael-valdron
Copy link
Member

@michael-valdron michael-valdron commented Apr 11, 2024 •
edited
Loading

Please specify the area for this PR

operator

What does does this PR do / why we need it:

Adds the fully qualified app name found under the helm chart and refactors to match usage. Additionally adds truncation to both app name and fully qualified app name as seen in the helm chart.

Which issue(s) this PR fixes:

Fixes #?

fixes devfile/api#1441, devfile/api#1442

PR acceptance criteria:

  • Test Coverage
    • Are your changes sufficiently tested, and are any applicable test cases added or updated to cover your changes?
  • Gosec scans

Documentation

  • Does the registry operator documentation need to be updated with your changes?

How to test changes / Special notes to the reviewer:

A reviewer will need to run these commands to run the integration tests locally or else the make test-integration will likely fail:

  1. Run make install-cert
  2. Wait for cert-manager to be ready, its under the cert-manager namespace
  3. Run make install && make deploy
  4. Wait for registry-operator to be ready, its under the registry-operator-system namespace
  5. Run make test-integration

@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Apr 11, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

This was referenced Apr 11, 2024
Closed
Closed
@michael-valdron
Copy link
Member Author

Review status is waiting to resolve an issue discovered during testing: devfile/api#1441 (comment)

@michael-valdron
app name functions and refactored devfile registry labels function fo…
6abf267 
…r app name getter changes

Signed-off-by: Michael Valdron <[email protected]>
@michael-valdron
truncateName & LabelsForDevfileRegistry test cases
04cca05 
Signed-off-by: Michael Valdron <[email protected]>
@michael-valdron
app name getter test cases
77f3ba8 
Signed-off-by: Michael Valdron <[email protected]>
@michael-valdron
refactor naming.go functions to use fully qualified app name instead …
f619305 
…of CR name

Signed-off-by: Michael Valdron <[email protected]>
@michael-valdron
change truncated length of app full name part of configmap name to ad…
3770d30 
…d space for suffix part

Signed-off-by: Michael Valdron <[email protected]>
@michael-valdron
naming function getter test cases
672ea32 
Signed-off-by: Michael Valdron <[email protected]>
@michael-valdron
truncateName & truncateNameLengthN comment blocks
02b528f 
Signed-off-by: Michael Valdron <[email protected]>
@michael-valdron
k8s ingress host name uses app full name rather than CR name to match…
8582b42 
… OpenShift route url

Signed-off-by: Michael Valdron <[email protected]>
@michael-valdron
Copy link
Member Author

Does the registry operator documentation need to be updated with your changes?

Out of issue scope.

@michael-valdron michael-valdron marked this pull request as ready for review April 12, 2024 15:04
@openshift-ci openshift-ci bot requested review from elsony and johnmcollier April 12, 2024 15:04
@michael-valdron michael-valdron changed the title [WIP]Feature: Fully Qualified App Name Feature: Fully Qualified App Name Apr 12, 2024
@Jdubrick
Copy link
Contributor

Unit tests are passing for me - Waiting on a cluster to spin up so I can run the integration tests because they don't want to work on my local crc

@michael-valdron
Copy link
Member Author

Unit tests are passing for me - Waiting on a cluster to spin up so I can run the integration tests because they don't want to work on my local crc

@Jdubrick Updated the reviewer notes on the PR description with the steps to ensure make test-integration runs properly.

@Jdubrick
Copy link
Contributor

/lgtm

All unit and integration tests are passing for me. I'm okay with it being merged but if @thepetk wants to take a look before that's okay with me.

@openshift-ci openshift-ci bot added the lgtm label Apr 12, 2024
thepetk
thepetk reviewed Apr 15, 2024
View reviewed changes
Copy link
Contributor

@thepetk thepetk left a comment

Choose a reason for hiding this comment

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

Overall looks good to me, left only a minor question

pkg/registry/constants.go Show resolved Hide resolved
@michael-valdron michael-valdron requested a review from thepetk April 15, 2024 13:48
thepetk
thepetk approved these changes Apr 15, 2024
View reviewed changes
Copy link
Contributor

@thepetk thepetk left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Apr 15, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: michael-valdron, thepetk

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [michael-valdron,thepetk]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@michael-valdron michael-valdron merged commit 89120e9 into devfile:main Apr 15, 2024
9 checks passed
thepetk pushed a commit to thepetk/devfile-registry-operator that referenced this pull request Aug 20, 2024
@michael-valdron @thepetk
Feature: Fully Qualified App Name (devfile#86)
df390d9 
* app name functions and refactored devfile registry labels function for app name getter changes

Signed-off-by: Michael Valdron <[email protected]>

* truncateName & LabelsForDevfileRegistry test cases

Signed-off-by: Michael Valdron <[email protected]>

* app name getter test cases

Signed-off-by: Michael Valdron <[email protected]>

* refactor naming.go functions to use fully qualified app name instead of CR name

Signed-off-by: Michael Valdron <[email protected]>

* change truncated length of app full name part of configmap name to add space for suffix part

Signed-off-by: Michael Valdron <[email protected]>

* naming function getter test cases

Signed-off-by: Michael Valdron <[email protected]>

* truncateName & truncateNameLengthN comment blocks

Signed-off-by: Michael Valdron <[email protected]>

* k8s ingress host name uses app full name rather than CR name to match OpenShift route url

Signed-off-by: Michael Valdron <[email protected]>

* fix full name contains case

Signed-off-by: Michael Valdron <[email protected]>

* fix given CR names in integration test cases to contain default app name devfile-registry

Signed-off-by: Michael Valdron <[email protected]>

---------

Signed-off-by: Michael Valdron <[email protected]>
Signed-off-by: thepetk <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thepetk thepetk thepetk approved these changes

@elsony elsony Awaiting requested review from elsony

@johnmcollier johnmcollier Awaiting requested review from johnmcollier

@Jdubrick Jdubrick Awaiting requested review from Jdubrick

Assignees

@thepetk thepetk

@Jdubrick Jdubrick

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Registry operator full app name support
3 participants
@michael-valdron @Jdubrick @thepetk