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

Rollback 3187 #3248

Merged
Merged

Conversation

foolusion
Copy link
Contributor

Description: Rollback #3187

Fix #3242 with rollback of 3187. Please make sure this change works for clusters with custom domains before re submitting.

Link to tracking Issue(s): #3242

  • Resolves: #issue-number

Testing: This rolls back to previously working code.

Documentation: None

@foolusion foolusion requested a review from a team August 30, 2024 17:08
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.


# One or more tracking issues related to the change
issues: [3160]
issues: [3242]
Copy link
Member

Choose a reason for hiding this comment

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

I think it would make sense to create a new changelog entry for the rollback.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I'm unfamiliar with the process. I think I've done it correctly now, but let me know if I missed something.

@jaronoff97
Copy link
Contributor

this lgtm @foolusion can you resolve the conflicts so we can merge this prior to release?

# Conflicts:
#	controllers/builder_test.go
@foolusion foolusion force-pushed the fix-3242-rollback-of-3187 branch from d4516e2 to b065f91 Compare September 5, 2024 01:46
@foolusion
Copy link
Contributor Author

👍 Done

Copy link
Contributor

@swiatekm swiatekm left a comment

Choose a reason for hiding this comment

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

LGTM

component: target allocator

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: Rollback 3187
Copy link
Contributor

Choose a reason for hiding this comment

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

We should at least do #3187, so this renders into a link to the PR in the changelog.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I wasn't sure about that since I thought it might consider it a line comment. Looks like it already got merged.

@jaronoff97 jaronoff97 merged commit dfbf0b8 into open-telemetry:main Sep 5, 2024
33 checks passed
@jaronoff97
Copy link
Contributor

thank you for your contribution 🙇

@foolusion foolusion deleted the fix-3242-rollback-of-3187 branch September 5, 2024 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TargetAllocator broken after release 0.105 when using custom domains.
4 participants