api: deprecated version annotation introduction by adisuissa · Pull Request #15564 · envoyproxy/envoy

adisuissa · 2021-03-19T02:01:10Z

Commit Message: api: deprecated version annotation introduction
Additional Description:
Adding a deprecated API version annotation to deprecated fields and enum values in proto files.
This is part of the work on adding minor/patch versioning work.

Risk Level: Low (adding annotation to existing protos).
Testing: Added and modified tests for the tooling (in tools/testdata).
Docs Changes:
Release Notes:
Platform Specific Features: None

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

repokitteh-read-only · 2021-03-19T02:01:20Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
API shepherd assignee is @htuch
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #15564 was opened by adisuissa.

see: more, trace.

adisuissa · 2021-03-19T02:03:24Z

The first commit 3ffd898 has the relevant scripts changes (including tests).
The second commit 8f9b997 contains the updates to all protos in api and generated_api_shadow.

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

adisuissa · 2021-03-19T16:48:04Z

/retest

repokitteh-read-only · 2021-03-19T16:48:09Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #15564 (comment) was created by @adisuissa.

see: more, trace.

htuch

This is really great @adisuissa, matches exactly what we need. @phlax could you take a pass over this from a Python & tools/ perspective?
/wait

tools/api_versioning/generate_api_version_header_test.py

tools/api_versioning/utils.py

tools/protoxform/protoprint.py

phlax

hi @adisuissa

this PR will need a rebase as i have been making quite invasive changes to the py code - apologies for that

overall looks like a very welcome addition - im wondering if we can/should use the rst .. deprecated:: directive - but i guess that is beyond the scope of this PR

tools/api_versioning/generate_api_version_header_test.py

tools/api_versioning/utils.py

tools/protoxform/protoprint.py

…annotation_v2 Signed-off-by: Adi Suissa-Peleg <adip@google.com>

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

…annotation_v2 Signed-off-by: Adi Suissa-Peleg <adip@google.com>

adisuissa · 2021-03-23T02:28:55Z

@htuch, @phlax thanks for the comments!
It should be all set now.

adisuissa · 2021-03-23T13:45:18Z

/retest

repokitteh-read-only · 2021-03-23T13:45:22Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #15564 (comment) was created by @adisuissa.

see: more, trace.

phlax

thanks for update - comments inline

phlax · 2021-03-23T13:48:05Z

tools/api_versioning/generate_api_version_header_test.py

 """
 import generate_api_version_header
-from generate_api_version_header import ApiVersion
+import utils


could you move this below the std library imports

phlax · 2021-03-23T13:49:21Z

tools/api_versioning/utils.py

+    """Returns the API version from a given API version input file.
+
+  Args:
+    input_path: the file containing the API version (API_VERSION).


these docstring still need further indenting

phlax · 2021-03-23T13:51:12Z

tools/protoxform/protoprint.py


 from tools.api_proto_plugin import annotations, traverse, visitor
+import tools.api_versioning.utils as api_version_utils
 from tools.protoxform import options as protoxform_options, utils


and move this up one - in general its (sylistically) preferred that from ... import comes after import ... from the same module

@phlax can we have tooling to automate this? We have custom Python to do this for C++ to enforce Envoy opinionated include order.

isort is the goto for this altho it might fight with yapf (i think i saw something about compatibility tho)

phlax · 2021-03-23T13:57:23Z

tools/protoxform/protoprint.py

+                not protoxform_options.has_hide_option(field_or_evalue.options):
+            # If the field or enum value has annotation from deprecation.proto, need to import it.
+            if field_or_evalue.options.HasExtension(deprecation_tag) or \
+                    field_or_evalue.options.HasExtension(disallowed_tag):


think this would be more readable as:

self._needs_deprecation_annotation_import = ( field_or_evalue.options.HasExtension(deprecation_tag) or field_or_evalue.options.HasExtension(disallowed_tag))

similar above although in that case you would need to set a local var - eg

should_do_something = ( field_or_evalue.options.deprecated and not self._frozen_proto and not protoxform_options.has_hide_option(field_or_evalue.options)

(not sure on the best name for should_do_something - but in a way that is what i want to know)

This would then set/reset the flag on each invocation (not a flag that is only raised once).
I can do the following:

self._needs_deprecation_annotation_import = ( self._needs_deprecation_annotation_import or field_or_evalue.options.HasExtension(deprecation_tag) or field_or_evalue.options.HasExtension(disallowed_tag))

Do you think it's more understandable this way?

…annotation_v2 Signed-off-by: Adi Suissa-Peleg <adip@google.com>

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

htuch · 2021-03-25T13:24:54Z

@adisuissa can you merge main? @phlax I think we should stop merging format fixes until this PR merges.

…annotation_v2 Signed-off-by: Adi Suissa-Peleg <adip@google.com>

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

htuch

Looks good generally! Couple of minor comments..

htuch · 2021-03-25T13:31:39Z

tools/api_versioning/utils.py

+      a namedtuple containing the major, minor, patch versions.
+    """
+    lines = pathlib.Path(input_path).read_text().splitlines()
+    assert len(lines) == 1


Arguably we shouldn't be asserting on external conditions like file contents, but depends on how "internal" the file is.

also if this triggers it will raise an assertion error ~~about lengh~~ which might not be that helpful

I think an assertion/exception is needed in case the API_VERSION file contents are accidentally modified.
I've added a comment to the assertion failure, but let me know if you think it should be removed.

if you keep it, then add an exception that will be useful when the code runs

with just an assert you would get something like this

>>> assert len([]) == 1 Traceback (most recent call last): File "<stdin>", line 1, in <module> AssertionError

ie no useful info

i think you need to use an error that will provide info, eg:

if len(lines) != 1: raise APIHeaderError("Explanation of what has gone wrong")

not sure exactly what kind of error or message is best

Hmmm, interesting...
I was under the impression that assertion failure also prints the line number. This is what I get at the moment:

Traceback (most recent call last): File "/home/adip/git/envoy/bazel-bin/tools/protoxform/protoprint.runfiles/envoy/tools/protoxform/protoprint.py", line 733, in <module> traverse.traverse_file(file_proto, ProtoFormatVisitor(api_version_file_path, frozen_proto))) File "/home/adip/git/envoy/bazel-bin/tools/protoxform/protoprint.runfiles/envoy/tools/protoxform/protoprint.py", line 586, in __init__ current_api_version = api_version_utils.get_api_version(api_version_file_path) File "/home/adip/git/envoy/bazel-bin/tools/protoxform/protoprint.runfiles/envoy/tools/api_versioning/utils.py", line 28, in get_api_version assert len(lines) == 1, 'Api version file must have a single line of format X.Y.Z, where X, Y, and Z are non-negative integers.' AssertionError: Api version file must have a single line of format X.Y.Z, where X, Y, and Z are non-negative integers.

I don't object using an exception, although there are other errors that may emit an assertion failure (e.g., empty lines), so I'll just add explicit handling of these.

yep - use a proper exception - its also catchable then

htuch · 2021-03-25T13:35:40Z

tools/protoxform/protoprint.py

        file_proto: FileDescriptorProto for file.
        empty_file: are there no message/enum/service defs in file?
+        import_deprecation_proto: should the "envoy/annotation/deprecation.proto" be imported?
+        frozen_proto: is the proto file frozen?


The logic here is a bit hard to follow; could we have something more descriptive, e.g. has_minor_version_deprecation and then infer these other properties?

htuch · 2021-03-25T13:36:01Z

tools/protoxform/protoprint.py

+            return False
+    except ValueError:
+        return False
+    return True


Should this belong in utils? Good candidate for unit tests.

Moved, and added tests.

phlax · 2021-03-25T14:17:21Z

@phlax I think we should stop merging format fixes until this PR merges.

yep k - apologies @adisuissa - py formatting has been a bit of shifting target and been a merge/rebase nightmare

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

…annotation_v2 Signed-off-by: Adi Suissa-Peleg <adip@google.com>

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

phlax · 2021-03-26T16:52:57Z

docs are rendered here:

https://storage.googleapis.com/envoy-pr/15564/docs/index.html

maybe i misunderstand what is included here - or the purpose - but i expected to find some change to the docs re deprecation

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

…annotation_v2 Signed-off-by: Adi Suissa-Peleg <adip@google.com>

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

adisuissa · 2021-03-29T04:33:21Z

@phlax I think we should stop merging format fixes until this PR merges.

yep k - apologies @adisuissa - py formatting has been a bit of shifting target and been a merge/rebase nightmare

No worries, I can fix the format if needed (it would just be easier to see the relevant changes if there were no formatting changes needed).

docs are rendered here:

https://storage.googleapis.com/envoy-pr/15564/docs/index.html

maybe i misunderstand what is included here - or the purpose - but i expected to find some change to the docs re deprecation

My plan is to add an explanation to api/API_VERSIONING.md (see draft: #14943).
I can also update the docs, but not sure where. It seems that the API versioning points to the internal doc.
Another possibility is to add the explanation to this FAQ.

phlax · 2021-03-29T07:57:36Z

My plan is to add an explanation to api/API_VERSIONING.md

if we have a way of saying "this feature is deprecated in version X" it would be really helpful to include this in the docs that end users read - im still not sure if i understand the purpose exactly - or whether this provides that

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

phlax · 2021-03-29T19:23:48Z

/retest

repokitteh-read-only · 2021-03-29T19:23:52Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #15564 (comment) was created by @phlax.

see: more, trace.

htuch

LGTM, thanks! @phlax if you're good we can merge this.

htuch · 2021-03-30T00:11:37Z

tools/api_versioning/utils.py

+      input_path: the file containing the API version (API_VERSION).
+
+    Returns:
+      a namedtuple containing the major, minor, patch versions.


Nit: Throws docsstring

Added the Raises clause to the docstring.

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

…annotation_v2 Signed-off-by: Adi Suissa-Peleg <adip@google.com>

adisuissa · 2021-03-30T04:01:02Z

if we have a way of saying "this feature is deprecated in version X" it would be really helpful to include this in the docs that end users read - im still not sure if i understand the purpose exactly - or whether this provides that

I agree that the docs should be updated, once the patch version capabilities will be added (until now the work has mainly added minor version capabilities).
Moreover, we'll need to clarify which deprecated fields are no longer supported in a specific version.

@phlax thanks for the thorough review and great comments!

phlax

lgtm, thanks @adisuissa

im looking forward to getting a better understanding of how we can use in docs

phlax · 2021-03-30T19:22:50Z

/retest

repokitteh-read-only · 2021-03-30T19:22:53Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #15564 (comment) was created by @phlax.

see: more, trace.

htuch

LGTM, thanks!

adisuissa added 2 commits March 18, 2021 21:34

api: Introducing deprecated version annotation

3ffd898

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

Applying change to all api protos

8f9b997

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

repokitteh-read-only bot added the api label Mar 19, 2021

repokitteh-read-only bot assigned htuch Mar 19, 2021

adisuissa added 2 commits March 18, 2021 22:19

Fixing functions casing

6245f45

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

Removing unneeded readme file

8898d79

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

htuch suggested changes Mar 22, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Mar 22, 2021

htuch assigned phlax Mar 22, 2021

phlax requested changes Mar 22, 2021

View reviewed changes

adisuissa added 3 commits March 22, 2021 10:01

Merge remote-tracking branch 'upstream/main' into deprecated_version_…

478e47c

…annotation_v2 Signed-off-by: Adi Suissa-Peleg <adip@google.com>

Addressing comments

26e61a8

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

Merge remote-tracking branch 'upstream/main' into deprecated_version_…

180bf47

…annotation_v2 Signed-off-by: Adi Suissa-Peleg <adip@google.com>

repokitteh-read-only bot removed the waiting label Mar 23, 2021

phlax requested changes Mar 23, 2021

View reviewed changes

adisuissa added 2 commits March 23, 2021 23:22

Merge remote-tracking branch 'upstream/main' into deprecated_version_…

3b90fbc

…annotation_v2 Signed-off-by: Adi Suissa-Peleg <adip@google.com>

Addressing comments

559518f

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

adisuissa added 2 commits March 25, 2021 09:34

Merge remote-tracking branch 'upstream/main' into deprecated_version_…

aada1df

…annotation_v2 Signed-off-by: Adi Suissa-Peleg <adip@google.com>

Format

8b83c10

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

htuch suggested changes Mar 25, 2021

View reviewed changes

Remove unneeded imports

aea76fc

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

adisuissa added 3 commits March 25, 2021 15:24

Merge remote-tracking branch 'upstream/main' into deprecated_version_…

214142d

…annotation_v2 Signed-off-by: Adi Suissa-Peleg <adip@google.com>

Merge remote-tracking branch 'upstream/main' into deprecated_version_…

607ab48

…annotation_v2 Signed-off-by: Adi Suissa-Peleg <adip@google.com>

Applying to latest proto changes

3a7e158

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

adisuissa added 4 commits March 28, 2021 23:38

Addressing comments

655e492

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

Merge remote-tracking branch 'upstream/main' into deprecated_version_…

19b4c0f

…annotation_v2 Signed-off-by: Adi Suissa-Peleg <adip@google.com>

Fix format

1f5154d

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

Fix format

7936f5b

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

Replacing assertion with exception

05a7f37

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

htuch previously approved these changes Mar 30, 2021

View reviewed changes

repokitteh-read-only bot removed the api label Mar 30, 2021

adisuissa added 2 commits March 29, 2021 23:50

Adding Raises docstring

b132df1

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

Merge remote-tracking branch 'upstream/main' into deprecated_version_…

051cf1a

…annotation_v2 Signed-off-by: Adi Suissa-Peleg <adip@google.com>

adisuissa dismissed htuch’s stale review via 051cf1a March 30, 2021 03:57

repokitteh-read-only bot added the api label Mar 30, 2021

phlax approved these changes Mar 30, 2021

View reviewed changes

phlax mentioned this pull request Mar 30, 2021

In docs, make use of version_added, version_changed, deprecated etc sphinx directives #15763

Open

htuch approved these changes Mar 31, 2021

View reviewed changes

repokitteh-read-only bot removed the api label Mar 31, 2021

htuch merged commit 5a8bfa2 into envoyproxy:main Mar 31, 2021

Conversation

adisuissa commented Mar 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Mar 19, 2021

Uh oh!

adisuissa commented Mar 19, 2021

Uh oh!

adisuissa commented Mar 19, 2021

Uh oh!

repokitteh-read-only bot commented Mar 19, 2021

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

phlax left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adisuissa commented Mar 23, 2021

Uh oh!

adisuissa commented Mar 23, 2021

Uh oh!

repokitteh-read-only bot commented Mar 23, 2021

Uh oh!

phlax left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch commented Mar 25, 2021

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phlax Mar 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phlax Mar 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

adisuissa commented Mar 19, 2021 •

edited

Loading

phlax Mar 26, 2021 •

edited

Loading

phlax Mar 29, 2021 •

edited

Loading