8000 feat(sumologicextension): deprecate install_token in favor of installation_token by sumo-drosiek · Pull Request #969 · SumoLogic/sumologic-otel-collector · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

feat(sumologicextension): deprecate install_token in favor of installation_token #969

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 6 commits into from
Feb 14, 2023
Merged

feat(sumologicextension): deprecate install_token in favor of installation_token #969

merged 6 commits into from
Feb 14, 2023

Conversation

sumo-drosiek
Copy link
Contributor

Signed-off-by: Dominik Rosiek drosiek@sumologic.com

@sumo-drosiek sumo-drosiek requested a review from a team as a code owner February 13, 2023 09:11
@github-actions github-actions bot added go documentation Improvements or additions to documentation labels Feb 13, 2023
@sumo-drosiek sumo-drosiek linked an issue Feb 13, 2023 that may be closed by this pull request
Change install_token in extension to installation_token #965
Closed
This was referenced Feb 13, 2023
Closed
Merged
@sumo-drosiek sumo-drosiek changed the title feat(sumologicextension): deprecate install_token feat(sumologicextension): deprecate install_token in favor od installtion_token Feb 13, 2023
@sumo-drosiek sumo-drosiek changed the title feat(sumologicextension): deprecate install_token in favor od installtion_token feat(sumologicextension): deprecate install_token in favor of installtion_token Feb 13, 2023
@andrzej-stencel
Copy link
Contributor

Should we use a feature gate for this breaking change? This is recommended by upstream here: https://github.com/open-telemetry/opentelemetry-collector/blob/v0.71.0/CONTRIBUTING.md#breaking-changes.

When deprecating a feature affecting end-users, consider first deprecating the feature in one version, then hiding it behind a feature flag in a subsequent version, and eventually removing it after yet another version.

@sumo-drosiek
Copy link
Contributor Author
sumo-drosiek commented Feb 13, 2023 •
edited
Loading

Should we use a feature gate for this breaking change? This is recommended by upstream here: https://github.com/open-telemetry/opentelemetry-collector/blob/v0.71.0/CONTRIBUTING.md#breaking-changes.

When deprecating a feature affecting end-users, consider first deprecating the feature in one version, then hiding it behind a feature flag in a subsequent version, and eventually removing it after yet another version.

Technically this change itself is not a breaking change. Removing install_token will be a breaking change, and we can do it with feature gate

I don't want to do a rename as a breaking change, because there will be problem with synchronising this change with UI, install script and documentation changes.

Dominik Rosiek added 3 commits February 14, 2023 15:11
@sumo-drosiek
feat: deprecate install_token
93f8634 
Signed-off-by: Dominik Rosiek <drosiek@sumologic.com>
@sumo-drosiek
chore(changelog): update
5ead0a2 
Signed-off-by: Dominik Rosiek <drosiek@sumologic.com>
@sumo-drosiek
fix: lint
a0e2820 
Signed-off-by: Dominik Rosiek <drosiek@sumologic.com>
@sumo-drosiek sumo-drosiek force-pushed the drosiek-deprecate-install-token branch from 94d3932 to a0e2820 Compare February 14, 2023 14:11
@andrzej-stencel
Copy link
Contributor

We decided to not use a feature flag. Changing the config option install_token to installation_token is probably even easier than adding a feature flag CLI option to the binary invocation, so it doesn't seem to be worth it.

andrzej-stencel
andrzej-stencel reviewed Feb 14, 2023
View reviewed changes
Copy link
Contributor
@andrzej-stencel andrzej-stencel left a comment

Choose a reason for hiding this comment

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

So you're adding a new configuration option installation_token but not updating the docs? 🤔

docs: update package documentation related to install_token
62dc5d7 
Signed-off-by: Dominik Rosiek <drosiek@sumologic.com>
sumo-drosiek
sumo-drosiek commented Feb 14, 2023
View reviewed changes
sumo-drosiek and others added 2 commits February 14, 2023 15:29
@sumo-drosiek sumo-drosiek changed the title feat(sumologicextension): deprecate install_token in favor of installtion_token feat(sumologicextension): deprecate install_token in favor of installation_token Feb 14, 2023
@sumo-drosiek sumo-drosiek enabled auto-merge (squash) February 14, 2023 14:35
@sumo-drosiek sumo-drosiek 7AB0 merged commit 6287bf5 into main Feb 14, 2023
@sumo-drosiek sumo-drosiek deleted the drosiek-deprecate-install-token branch February 14, 2023 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrzej-stencel andrzej-stencel andrzej-stencel approved these changes

Assignees
No one assigned
Labels
documentation Improvements or additions to documentation go
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Change install_token in extension to installation_token
2 participants
@sumo-drosiek @andrzej-stencel
0