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

Remove deprecated annotations #873

Merged
merged 4 commits into from
Sep 29, 2020
Merged

Conversation

rainest
Copy link
Contributor

@rainest rainest commented Sep 23, 2020

What this PR does / why we need it:
Removes deprecated annotations.

Special notes for your reviewer:
Tests still use literal "konghq.com/regex-priority": 3 style test values instead of AnnotationPrefix + RegexPriorityKey: 3 package constants. We're kinda moving towards the latter in general, though you could make the argument that we want the literal strings so tests complain in the event that we change something: while that works in code using the constants, users won't have changed their annotations in advance if we release such a change, so those literals could serve as a useful canary to make sure that we call out breaking changes if we do change these in the future.

Several tests outside the annotations package referenced annotations.DeprecatedPluginsKey and now instead reference annotations.AnnotationPrefix + annotations.PluginsKey. For consistency, and lack of an obvious reason not to, the annotations package now exports both the modern prefix and all keys, even though not all keys are currently used in non-annotation tests.

@rainest rainest added this to the 1.0.0 milestone Sep 23, 2020
@rainest rainest requested a review from hbagdi September 23, 2020 18:54
@rainest rainest force-pushed the feat/remove-deprecated-annotations branch from cbc8279 to 1fc8384 Compare September 23, 2020 18:57
@hbagdi
Copy link
Member

hbagdi commented Sep 25, 2020

Looks good, deferring approval to @mflendrich.

mflendrich
mflendrich previously approved these changes Sep 29, 2020
Copy link
Contributor

@mflendrich mflendrich left a comment

Choose a reason for hiding this comment

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

LGTM but please fix the dead link (see my other comment).

that we want the literal strings so tests complain in the event that we change something

Testing our application's contract (in particular, that user-facing annotations work as expected by users) sounds great. I think that a good place for contract testing would be on a higher level than here, though - otherwise we're likely to implement a change-detector test with all the problems as mentioned in this ToTT blog post.

I think that the end-to-end test (see #869) could be a good start for such contract tests.

docs/guides/upstream-mtls.md Outdated Show resolved Hide resolved
Co-authored-by: Michał Flendrich <michal@flendrich.pro>
@hbagdi hbagdi merged commit 41f9fbf into next Sep 29, 2020
@hbagdi hbagdi deleted the feat/remove-deprecated-annotations branch September 29, 2020 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants