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

[chore] Refactor platform to be openshift routes availability #1463

Merged
merged 7 commits into from
Feb 17, 2023

Conversation

iblancasa
Copy link
Contributor

Closes #1418.

Replace the OpenShift platform detection with OpenShift Routes API detection.

@iblancasa iblancasa requested a review from a team February 9, 2023 10:26
Copy link
Member

@frzifus frzifus left a comment

Choose a reason for hiding this comment

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

Cool, only 2 small things ^^

pkg/autodetect/main.go Outdated Show resolved Hide resolved
pkg/autodetect/openshiftroutes.go Outdated Show resolved Hide resolved
@jaronoff97
Copy link
Contributor

@iblancasa could you remove the [chore] in the title and add a changelog entry please? this seems to be a bit more than a minor version upgrade / test fix.

Signed-off-by: Israel Blancas <[email protected]>
@iblancasa iblancasa changed the title [chore] Refactor platform to be openshift routes availability Refactor platform to be openshift routes availability Feb 9, 2023
component: operator

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: "Change the platform detection to be API detection. Previously, the operator was checking the plaform where it was running -OpenShift or Kubernetes-. Now, we just check if the OpenShift Route API is enabled"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
note: "Change the platform detection to be API detection. Previously, the operator was checking the plaform where it was running -OpenShift or Kubernetes-. Now, we just check if the OpenShift Route API is enabled"
note: "Change the platform detection to be API detection. Previously, the operator was checking the plaform where it was running OpenShift or Kubernetes. Now, we just check if the OpenShift Route API is enabled"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this change lefts the sentence with a different meaning or not well written. I would prefer to keep how it was written

Copy link
Member

Choose a reason for hiding this comment

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

This does not need to be in the changelog. it's an internal refactoring.

component: operator

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: "Change the platform detection to be API detection. Previously, the operator was checking the plaform where it was running -OpenShift or Kubernetes-. Now, we just check if the OpenShift Route API is enabled"
Copy link
Member

Choose a reason for hiding this comment

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

This does not need to be in the changelog. it's an internal refactoring.

pkg/autodetect/main.go Outdated Show resolved Hide resolved
pkg/autodetect/main.go Outdated Show resolved Hide resolved
@pavolloffay
Copy link
Member

@iblancasa could you remove the [chore] in the title and add a changelog entry please? this seems to be a bit more than a minor version upgrade / test fix.

@jaronoff97 do we need to add a changelog for these "internal" changes?

@pavolloffay
Copy link
Member

The code is exposed, but I think we should consider making most of the packages internal as they were not designed to be imported externally.

Signed-off-by: Israel Blancas <[email protected]>
@jaronoff97
Copy link
Contributor

@pavolloffay I agree, if these were internal only, i don't think the changelog would be necessary. Given these have the potential to be used externally right now, I was thinking it would be valuable to have the entry.

@iblancasa iblancasa changed the title Refactor platform to be openshift routes availability [chore] Refactor platform to be openshift routes availability Feb 13, 2023
@iblancasa
Copy link
Contributor Author

Changelog removed

@pavolloffay pavolloffay merged commit 55dd334 into open-telemetry:main Feb 17, 2023
ItielOlenick pushed a commit to ItielOlenick/opentelemetry-operator that referenced this pull request May 1, 2024
…elemetry#1463)

* Refactor platform to be openshift routes availability

* Add minor changes asked in code review

Signed-off-by: Israel Blancas <[email protected]>

* Add changelog

Signed-off-by: Israel Blancas <[email protected]>

* Rename method

Signed-off-by: Israel Blancas <[email protected]>

* Delete changelog as is not needed

Signed-off-by: Israel Blancas <[email protected]>

---------

Signed-off-by: Israel Blancas <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants