Skip to content

Restore Assist file conf options for non-breaking deprecation #43301

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

Closed
wants to merge 1 commit into from

Conversation

Joerger
Copy link
Contributor

@Joerger Joerger commented Jun 20, 2024

Fixes Teleport start error described here - #42657 (comment)

@Joerger Joerger added no-changelog Indicates that a PR does not require a changelog entry backport/branch/v16 labels Jun 20, 2024
@Joerger Joerger requested review from jakule and GavinFrazar June 20, 2024 17:40
Copy link
Contributor

@jakule jakule left a comment

Choose a reason for hiding this comment

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

Can we merge this only in v16? Current master will be released as v17 so that PR will need to be reverted anyway.

@GavinFrazar
Copy link
Contributor

why mark it to delete in v17? it will just be the same issue?

@jakule
Copy link
Contributor

jakule commented Jun 20, 2024

why mark it to delete in v17?

I don't think it makes sense to leave the warning forever since assist is removed from Teleport.

it will just be the same issue?

Should we just fix the cloud config and not merge this PR? I agree that this only pushes the real issue for a few months.

@Joerger
Copy link
Contributor Author

Joerger commented Jun 20, 2024

I don't think it makes sense to leave the warning forever since assist is removed from Teleport.

There are several other cases in the file config where we keep a deprecated config field indefinitely to prevent unnecessary support load on misconfigurations. It would make sense to keep the option for at least 1 major version, if not longer.

Should we just fix the cloud config and not merge this PR? I agree that this only pushes the real issue for a few months.

@jakule Is assist only supported for cloud? If not, any customer that had Assist enabled in v15 would run into this issue on upgrade.

@GavinFrazar
Copy link
Contributor

self-hosted customers may have an assist config with empty values and will hit the issue, regardless of the feature flag being enabled. I think we should keep it for at least a few major versions, if not forever. We document that our config file is versioned, therefore a breaking change in config file format belongs in a new version - so we can delete this in config file v4 😉

In order to avoid breaking existing configurations, Teleport's configuration is
versioned. The newer configuration version is v3. If a version is not
specified in the configuration file, v1 is assumed.

Copy link
Contributor

@GavinFrazar GavinFrazar left a comment

Choose a reason for hiding this comment

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

approving since I only disagree on the TODO comment - my suggestion is to change that to "DELETE IN CONFIG FILE V4" instead of v17

@public-teleport-github-review-bot public-teleport-github-review-bot bot removed the request for review from EdwardDowling June 20, 2024 18:14
Copy link
Collaborator

@r0mant r0mant left a comment

Choose a reason for hiding this comment

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

Let's please not keep accumulating old flags, especially for such a niche feature. This is an intended breaking change in a major release and people are supposed to remove these fields from their configuration when upgrading.

The only thing we should've done better is include instructions for removing these fields in the breaking changes section in release notes. @jakule Can you please update it? And this PR I think we should just close.

@Joerger Joerger closed this Jun 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/branch/v16 no-changelog Indicates that a PR does not require a changelog entry size/sm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants