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

[Transform] Filter availability #2177

Merged
merged 15 commits into from
Jul 17, 2023
Merged

[Transform] Filter availability #2177

merged 15 commits into from
Jul 17, 2023

Conversation

Anaethelion
Copy link
Contributor

This PR adds a subscript to the transform list to filter the specification and types based on the @availability tags.

The script defaults to the standard schema.json and output a schema-filtered.json in the same directory based on the provided flags. (e.g. --stack or --serverless)
Input and output path the different source and target can be provided.

@Anaethelion Anaethelion requested a review from a team as a code owner July 12, 2023 09:03
@Anaethelion Anaethelion marked this pull request as draft July 12, 2023 09:06
@Anaethelion Anaethelion marked this pull request as ready for review July 12, 2023 10:44
Copy link
Member

@JoshMock JoshMock left a comment

Choose a reason for hiding this comment

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

Lots of nit-picky little JS type-checking things. Otherwise the change looks good to me. 👍

compiler/src/transform/filter-by-availability.ts Outdated Show resolved Hide resolved
compiler/src/transform/filter-by-availability.ts Outdated Show resolved Hide resolved
if (((availabilities.serverless != null) || (availabilities.stack != null)) && serverless && stack) {
return true
}
return false
Copy link
Member

Choose a reason for hiding this comment

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

I'm trying to line this logic up with the comment from @swallez on #2107:

If there's no @availability annotation, the endpoint or property is available in all environments.

In my serverless generator code, the filter for inclusion into the serverless client is much simpler, and defaults to true to recognize Sylvain's recommendation above:

function serverlessAvailability(item: M.Property | M.Endpoint): boolean {
  if (item.availability) {
    return item.availability?.serverless?.visibility === 'public'
  }
  return true
}

To extend that to also support stack availability like your function, I would do something like:

function availability(item: M.Property | M.Endpoint, stack: boolean, serverless: boolean): boolean {
  let isServerless = true
  let isStack = true

  if (item.availability) {
    isServerless = (item.availability?.serverless?.visibility === 'public')
    isStack = (item.availability?.stack?.visibility === 'public')
  }

  if (serverless && stack) {
    return isStack || isServerless
  } else if (serverless) {
    return isServerless
  } else if (stack) {
    return isStack
  }
}

This is more readable, to me at least. It also checks the string value of visibility. Based on my understanding, following Sylvain's comment above, the only way to exclude an item from the serverless spec is to include a serverless availability annotation marking it as private, e.g. @availability serverless stability=stable visibility=private.

Please correct me if I'm misunderstanding! The structure and implied defaults of the @availability tag values leave some things up to interpretation. It might be less confusing if the generated schema.json included default values for availability.{stack,serverless}.* to make the defaults explicit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah!

I'd prefer to keep visibility out of this script, different languages can have different use of that notion we could want to use that independently.

For the availability tag my understanding is a bit more complex because no tags means available everywhere, but presence of the tag is a direct exclusion if both are not defined.
That allows us to embed everything by default that is common to both APIs and later differentiate what could be stack or serverless only.

For example in the future we could have a public API that is specific to serverless with @availability serverless or only available to stack with @availability stack; the same logic goes for properties.

Copy link
Member

Choose a reason for hiding this comment

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

You both have good points.

Visibility is indeed an additional dimension. Private APIs should not be included in serverless clients we provide to the general public. They either expose things users should not care about or that may be removed from serverless in the future. Private APIs can however be part of internal clients built for Elastic products (e.g. Kibana).

What these internal clients look like is something we haven't discussed much, and their shape may depend on the target language. However, I think that ATM we should not care much about it: the Stack client includes everything and products such as Kibana already use it. So let just them keep using it and work on this later.

So back to this transformation, I think we should include visibility as an additional filtering parameter (with no default value to avoid implicit assumptions).

  • if there is some availablity defined, reject if the target flavor is not present (items with no availability defined are included everywhere)
  • if the visibility is private for the target flavor (Stack or Serverless), and the configuration is "public", reject the item
  • otherwise keep the item.

BTW currently availability is defined with two distinct fields in the metamodel. We may want to change this to a dictionary, since this is what it conceptually is. This allows "some" visibility to be just a length check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a two step filter for both availability and if present visibility.

Visibility is a comma separated list to allow for cases like public,feature_flag since the visibility is an enum.

I completely agree changing the metamodel would allow to simplify usages, I'll do that change in another PR once we're through with this one.

compiler/src/transform/filter-by-availability.ts Outdated Show resolved Hide resolved
compiler/src/transform/filter-by-availability.ts Outdated Show resolved Hide resolved
compiler/src/transform/filter-by-availability.ts Outdated Show resolved Hide resolved
compiler/src/transform/filter-by-availability.ts Outdated Show resolved Hide resolved
compiler/src/transform/filter-by-availability.ts Outdated Show resolved Hide resolved
if (((availabilities.serverless != null) || (availabilities.stack != null)) && serverless && stack) {
return true
}
return false
Copy link
Member

Choose a reason for hiding this comment

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

You both have good points.

Visibility is indeed an additional dimension. Private APIs should not be included in serverless clients we provide to the general public. They either expose things users should not care about or that may be removed from serverless in the future. Private APIs can however be part of internal clients built for Elastic products (e.g. Kibana).

What these internal clients look like is something we haven't discussed much, and their shape may depend on the target language. However, I think that ATM we should not care much about it: the Stack client includes everything and products such as Kibana already use it. So let just them keep using it and work on this later.

So back to this transformation, I think we should include visibility as an additional filtering parameter (with no default value to avoid implicit assumptions).

  • if there is some availablity defined, reject if the target flavor is not present (items with no availability defined are included everywhere)
  • if the visibility is private for the target flavor (Stack or Serverless), and the configuration is "public", reject the item
  • otherwise keep the item.

BTW currently availability is defined with two distinct fields in the metamodel. We may want to change this to a dictionary, since this is what it conceptually is. This allows "some" visibility to be just a length check.

Copy link
Member

@swallez swallez left a comment

Choose a reason for hiding this comment

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

LGTM

@Anaethelion Anaethelion merged commit 2ba9674 into main Jul 17, 2023
8 checks passed
@Anaethelion Anaethelion deleted the filter_availability branch July 17, 2023 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants