Deprecate rest.authorization-url in favor of oauth2-server-uri #962

ndrluis · 2024-07-24T23:42:23Z

The purpose of this PR is to follow the OAuth property standard as defined in the Java implementation and the REST Catalog specification: https://github.com/apache/iceberg/blob/main/open-api/rest-catalog-open-api.yaml#L144-L145.

Deprecation message about the oauth tokens behavior:

Deprecation message about the property:

pyiceberg/catalog/rest.py

Fokko · 2024-07-25T07:07:08Z

pyiceberg/catalog/rest.py

@@ -290,7 +293,14 @@ def url(self, endpoint: str, prefixed: bool = True, **kwargs: Any) -> str:

    @property
    def auth_url(self) -> str:
-        if url := self.properties.get(AUTH_URL):
+        if self.properties.get(DEPRECATED_AUTH_URL):


I think the logic is a bit more elaborate, see: https://github.com/apache/iceberg/pull/10603/files#diff-86450612dbe323d6d06cbc3846aa1913f042eaedadc0ca027c36bfbe08d3a46cR196

The initial purpose of this PR was not to warn about the /oauth/tokens endpoint deprecation, it was only to fix the configuration property, but I can add this.

Fokko

Thanks for updating the PyIceberg code here @ndrluis Could you update the docs as well?

ndrluis · 2024-07-25T14:47:56Z

The deprecated decorator adds a message that does not make sense for the properties deprecation. I will add a new function and change where is a property deprecation and are using the deprecated decorator.

ndrluis · 2024-07-25T14:55:35Z

@Fokko Done!

HonahX

@ndrluis Thanks for working on this! Left some comments about minor stuff. Overall it looks great!

pyiceberg/utils/deprecated.py

mkdocs/docs/how-to-release.md

HonahX · 2024-07-26T19:05:55Z

pyiceberg/utils/deprecated.py

+
+
+def deprecation_message(deprecated_in: str, removed_in: str, help_message: Optional[str]) -> None:
+    """Mark properties as deprecated.


Currently we only use this to deprecate properties. But in the future, we may use it to deprecate other things like parameters. How about just making a general comment here saying that adding this will thrown a deprecation warning?

HonahX · 2024-07-26T19:16:38Z

pyiceberg/catalog/rest.py

+        if not has_oauth_server_uri and (has_init_token or has_credential) and not has_sigv4_enabled:
+            deprecation_message(
+                deprecated_in="0.7.0",
+                removed_in="future Iceberg release",


I am a little worried that this may get lost since this is not a version number. How about we remove this in the next major release: 1.0.0?

I am following what is described in the Java implementation. In the deprecation proposal, it is aimed to be removed in version 1.8.0 or 2.0.0. I believe that 1.8.0 could be released before pyiceberg 1.0.0, but we cannot know if the endpoint will be removed in version 1.8.0.

So, I don't have a strong opinion; I am open to what you think is better.

Thanks for pointing this out!

I believe that 1.8.0 could be released before pyiceberg 1.0.0, but we cannot know if the endpoint will be removed in version 1.8.0.

I think we can update the version to a later one (e.g. 1.2.0) in the following releases. So, it looks good to aim for 1.0.0 now.

Another concern is that "future iceberg release" may add confusion since this could mean any version after 0.7.0. So, I prefer to at least include a version number in the removed_in message.

Would love to hear your thoughts on this!

Changed in 6e253c2

ndrluis · 2024-07-27T13:42:20Z

We have already started the vote for release 0.7.0. I believe we need to change the deprecated_in to 0.8.0.

Thoughts, @HonahX @sungwy?

sungwy · 2024-07-27T15:43:15Z

We have already started the vote for release 0.7.0. I believe we need to change the deprecated_in to 0.8.0.

Thoughts, @HonahX @sungwy?

Hi @ndrluis - that sounds like a good idea.

We have been working on making the release for 0.7.0 for a while, which already comes with many exciting features users have been waiting on. For the last few weeks, I have been mostly working on introducing critical bug fixes into this release so we can finally make it onto the other side 🙂

If it’s alright with you, could we update this and target this change for 0.8.0 as you suggested?

ndrluis · 2024-07-27T20:32:34Z

Updated deprecated version in commit 1039ac3

sungwy

LGTM! Thank you for working on this @ndrluis

HonahX

LGTM! Thanks @ndrluis !

Fokko

Thanks for fixing this @ndrluis

And thanks @sungwy and @HonahX for the review 🚀

Fokko · 2024-08-08T08:48:29Z

pyiceberg/utils/deprecated.py

            return func(*args, **kwargs)

        return new_func

    return decorator
+
+
+def deprecation_message(deprecated_in: str, removed_in: str, help_message: Optional[str]) -> None:


Nice addition, I like it a lot 👍

Fokko reviewed Jul 25, 2024

View reviewed changes

pyiceberg/catalog/rest.py Outdated Show resolved Hide resolved

Fokko reviewed Jul 25, 2024

View reviewed changes

ndrluis force-pushed the fix-oauth-uri branch from 05b430e to e6d0198 Compare July 25, 2024 14:45

ndrluis force-pushed the fix-oauth-uri branch from e6d0198 to 26e49be Compare July 25, 2024 14:53

ndrluis requested a review from Fokko July 25, 2024 14:55

HonahX reviewed Jul 26, 2024

View reviewed changes

ndrluis requested a review from HonahX July 26, 2024 19:47

HonahX mentioned this pull request Jul 27, 2024

Deprecate ADLFS prefix in favor of ADLS #961

Merged

sungwy added this to the PyIceberg 0.8.0 release milestone Jul 27, 2024

ndrluis force-pushed the fix-oauth-uri branch 2 times, most recently from 8cfd954 to 913ee61 Compare August 2, 2024 14:14

Deprecate rest.authorization-url in favor of oauth2-server-uri

0830fdc

ndrluis force-pushed the fix-oauth-uri branch from 913ee61 to 637eedd Compare August 2, 2024 14:14

Use deprecation_message instead of deprecated for property deprecation

6d39cb4

ndrluis force-pushed the fix-oauth-uri branch from 637eedd to 6d39cb4 Compare August 2, 2024 14:18

sungwy approved these changes Aug 3, 2024

View reviewed changes

HonahX approved these changes Aug 5, 2024

View reviewed changes

Fokko approved these changes Aug 8, 2024

View reviewed changes

Fokko merged commit 50077af into apache:main Aug 8, 2024
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate rest.authorization-url in favor of oauth2-server-uri #962

Deprecate rest.authorization-url in favor of oauth2-server-uri #962

ndrluis commented Jul 24, 2024 •

edited

Loading

Fokko Jul 25, 2024

ndrluis Jul 25, 2024

Fokko left a comment

ndrluis commented Jul 25, 2024

ndrluis commented Jul 25, 2024

HonahX left a comment

HonahX Jul 26, 2024

HonahX Jul 26, 2024

ndrluis Jul 26, 2024

HonahX Jul 26, 2024

ndrluis Jul 27, 2024

ndrluis commented Jul 27, 2024

sungwy commented Jul 27, 2024

ndrluis commented Jul 27, 2024

sungwy left a comment

HonahX left a comment

Fokko left a comment

Fokko Aug 8, 2024



		def deprecation_message(deprecated_in: str, removed_in: str, help_message: Optional[str]) -> None:
		"""Mark properties as deprecated.

Deprecate rest.authorization-url in favor of oauth2-server-uri #962

Deprecate rest.authorization-url in favor of oauth2-server-uri #962

Conversation

ndrluis commented Jul 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fokko left a comment

Choose a reason for hiding this comment

ndrluis commented Jul 25, 2024

ndrluis commented Jul 25, 2024

HonahX left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ndrluis commented Jul 27, 2024

sungwy commented Jul 27, 2024

ndrluis commented Jul 27, 2024

sungwy left a comment

Choose a reason for hiding this comment

HonahX left a comment

Choose a reason for hiding this comment

Fokko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ndrluis commented Jul 24, 2024 •

edited

Loading