Moved and renamed all Predicate names and Filter names for consistency and ease of use

[peterklijn]

When using Skipper as a Library, it is hard to find the constants that hold the various predicate and filter names:

• They are in different packages (including some predicates that live outside of the predicates package.
• The naming is inconsistent
  • Some are <predicate name>Name
  • Some are Name (and rely on the package name)
  • Some are FilterName or PredicateName (and rely not the package name)
  • Some contain part of the filter / predicate name, which make them hard to find
  • etc
• Some names are private constants or regular strings, and cannot be used at all (e.a. the Host predicate)

In this PR I moved all predicate and filter name constants to the predicates and filters packages, added constants for the predicates and filters that used string values.

By having all predicates and filters in the same place, with consistent naming, it makes it easier to use Predicates if you use Skipper as a Library and build routes using code.

In this PR all names are the same as the actual Predicate and Filter names in the documentation, ending with a Name suffix as it was the most common pattern.

The other reason of this PR is that I would like to continue with building helpers for Filters and Predicates (#1537), and would like to prevent a single massive PR, which is hard to review.
With this change out of the way, it makes the following changes smaller :)

[AlexanderYastrebov]

@peterklijn Hello, thank you for the contribution.

I can see the point of having all names in one place but I do not know why we have it like it is in the first place so lets have an opinion from other maintainers.

What is a reason to leave deprecated names (like AccessLogDisabledName) in place?

Existing codebases would break at compile time and need to be adjusted if they used moved constants.
I can not think about other downsides at the moment.

[peterklijn]

Thanks Alexander, happy to hear that you like it!

What is a reason to leave deprecated names (like AccessLogDisabledName) in place?

I actually moved them initially but the staticcheck didn't allow it as it detect calling deprecated constants from other package.
I tried to ignore the deprecated check in the makefile first, but that didn't work (and it feels wrong to disable the check) so I changed it back..

Existing codebases would break at compile time and need to be adjusted if they used moved constants.

Yeah that's the only issue I could think of as well.
I think that we could mitigate that a bit by writing a short migration guide in the release notes, wdyt?

[peterklijn]

Hi @AlexanderYastrebov, do you think this PR could get merged?

[peterklijn]

@AlexanderYastrebov and @szuecs, could this PR get merged please?

[szuecs]

@peterklijn does it break for library users?
I think it's not bad to have it in a central place, but we also need to be careful not to break the code of other people.

We can always merge it to the v1 branch, but right now no one is keeping it up to date, but I think we would do a rebase on master as soon as we have a need to do a v1 release.

[peterklijn]

Hi Sandor, thanks for your quick reply!

@peterklijn does it break for library users?
I think it's not bad to have it in a central place, but we also need to be careful not to break the code of other people.

That's a fair point. I guess technically it is a breaking change, as the public constants have been moved, and renamed for consistency. However it does not change the behaviour.

What are your policies around such changes? I was hoping it would be fine to update from 1.13.x. to 1.14.0. Have all minor (major.minor.patch) updates have been without such breaking changes?

If this PR was changing the behaviour, like how the Path predicate would behave for example, I fully agree it would be a breaking change and it should be a major update.

We can always merge it to the v1 branch, but right now no one is keeping it up to date, but I think we would do a rebase on master as soon as we have a need to do a v1 release.

This depends on when you're expecting to release a v1. I don't think this change justifies such a big release so it would stay on that branch for quite some time.

Alternative approach

If you don't think this change is acceptable for a minor release, I have an alternative solution and would like to get your opinion on it before I start working on it :)

I'm taking this file as an example: https://github.com/zalando/skipper/pull/1828/files#diff-66a56656328ff9986ec456542c48bda5aa5f21008b75dc64c15341a063001188

It changed from this:

const (
 	Name = "apiUsageMonitoring" // <-- removed

 	unknownPlaceholder = "{unknown}"
 	noMatchPlaceholder = "{no-match}"
 	noTagPlaceholder   = "{no-tag}"
 )

 var (
 	log      = logrus.WithField("filter", Name)
 	regCache = sync.Map{}
 )

To this:

const (
 	unknownPlaceholder = "{unknown}"
 	noMatchPlaceholder = "{no-match}"
 	noTagPlaceholder   = "{no-tag}"
 )

 var (
 	log      = logrus.WithField("filter", filters.ApiUsageMonitoringName) // <-- use constant from filters package
 	regCache = sync.Map{}
 )

Instead I can change it to this:

const (
	// Deprecated, use filters.ApiUsageMonitoringName instead
	Name = filters.ApiUsageMonitoringName // <-- Leave but mark as deprecated

 	unknownPlaceholder = "{unknown}"
 	noMatchPlaceholder = "{no-match}"
 	noTagPlaceholder   = "{no-tag}"
 )

 var (
 	log      = logrus.WithField("filter", filters.ApiUsageMonitoringName) // <-- keep using constant from filters package
 	regCache = sync.Map{}
 )

This way, it is no longer a breaking change, as all the old constants are still there, but because they're marked as deprecated so editors like GoLand will show users that they have to change.

WDYT?

[szuecs]

@peterklijn I like the idea, that you outlined in "Alternative approach". The bigger question about if this is a minor release increase, I can answer in my opinion:
If a user creates its own proxy based on skipper library and includes some of the standard predicates and filters and adds some custom ones themselves, then a change should not break the build.
In practice it would be sometimes fine to break the build even in these cases, for example if there is only a breaking change for a filter that is likely only internally used like apiMonitoring.
Normally for a change like this I would ask in slack and wait a week such that everyone can check and have an opinion if this breaks their build or not.

I would also like to have a buy-in by other maintainers like before I let you work too much on this topic. @AlexanderYastrebov @aryszka @marcinzaremba wdyt about the introduced change in this PR (it's many files but there are only filters/filters.go and predicate/predicates.go which are interesting and the others are all similar)?

[aryszka]

Hi @peterklijn ,

thanks a lot for the change. This refactoring makes a lot of sense to me, and many thanks for being considerate and splitting up the changes that you're planning. It was an easy review. ❤️

My principle regarding such changes is that we try not to make any assumptions on who and how they use our packages. This means that we assume that there can be people who use our packages in alternative ways and they cannot be contacted. We also assume such projects have automatic builds, so the release notes is not a completely clean solution. This also means that we always try to maintain backwards compatibility of the public interfaces of the packages.

With that said, to me your alternative suggestion is clearly the right way to go, (Name = filters.ApiUsageMonitoringName). If you could push that version, I'd be happy to merge.

Cheers,
Arpad

[peterklijn]

Hi @AlexanderYastrebov, @szuecs and @aryszka,
Thanks again for your reviews and comments. I made the discussed changes, now all public constants are back so it's no longer a breaking change.
Please have another look :)

Ps, I recommend that you hide whitespace changes to make the review a bit easier

[szuecs]

👍

[aryszka]

👍

[aryszka]

👍

[szuecs]

👍