refactor: change use of pyrfc3339 to use ciso8601 by EdmilsonRodrigues · Pull Request #1243 · juju/python-libjuju

EdmilsonRodrigues · 2024-12-17T06:02:20Z

Description

This change was mentioned in the issue #1101 . There is just one change that is not simply replacing, and that one is the generation of the rfc3339, but I created a simple function to handle this conversion.

\Replaced old pyrfc3339 that has not received an updated for the last 6 years and is not typed to use ciso8601 instead

QA Steps

<Commands / tests / steps to run to verify that the change works:>

tox -e py3 -- tests/unit/...

tox -e integration -- tests/integration/...

All CI tests need to pass.

<Please note that most likely an additional test will be required by the reviewers for any change that's not a one liner to land.>

Notes & Discussion

Should I also write the tests for this one? Like assert pyrfc3339.parse(timestamp) == ciso8601.parse_rfc339(timestamp) and assert pyrfc3339.generate(datetime) == generate_rfc3339_from_unix_time(datetime)?

dimaqq · 2024-12-17T06:37:34Z

What's your personal opinion on ciso8601 vs. backports.datetime_fromisoformat?
https://github.com/closeio/ciso8601/blob/master/why_ciso8601.md

dimaqq · 2024-12-17T06:38:41Z

Adding @Aflynn50 for the Juju perspective.

dimaqq · 2024-12-17T06:41:46Z

juju/client/gocookies.py

        # Note: fromtimestamp bizarrely produces a time without
        # a time zone, so we need to use accept_naive.
-        go_cookie["Expires"] = pyrfc3339.generate(unix_time, accept_naive=True)
+        go_cookie["Expires"] = generate_rfc3339_from_unix_time(unix_time)


I see that there's a solid-looking set of tests in tests/unit/test_gocookies.py.
Could you clarify why the custom parses function was needed, what specific inputs could not be parsed?

ciso8601 does not have a function to generate a rfc3339 timestamp from a datetime, as this one of the pyrfc3339 library, so I implemented my own.

I wonder if datetime.isoformat() would be enough.
I'll need someone who knows about go-style cookies and why are those important, if Juju relies on those in some way.

dimaqq · 2024-12-17T06:48:21Z

/build

dimaqq · 2024-12-17T07:58:35Z

Fixes #1101

dimaqq · 2024-12-17T07:59:41Z

________________________ TestGoCookieJar.test_roundtrip ________________________

self = <tests.unit.test_gocookies.TestGoCookieJar testMethod=test_roundtrip>

    def test_roundtrip(self):
        jar = self.load_jar(cookie_content)
        filename2 = os.path.join(self.dir, "cookies2")
>       jar.save(filename=filename2)

tests/unit/test_gocookies.py:201: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
juju/client/gocookies.py:46: in save
    go_cookies.append(py_to_go_cookie(cookie))
juju/client/gocookies.py:107: in py_to_go_cookie
    go_cookie["Expires"] = generate_rfc3339_from_unix_time(unix_time)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

unix_time = datetime.datetime(2345, 11, 15, 18, 16, 8)

    def generate_rfc3339_from_unix_time(unix_time: datetime.datetime) -> str:
>       rfc, discard = unix_time.isoformat().split(".")
E       ValueError: not enough values to unpack (expected 2, got 1)

Please run unit tests 🙇🏻
The easiest way is uvx tox -e unit

EdmilsonRodrigues · 2024-12-17T09:47:26Z

Re: What's your personal opinion on ciso8601 vs. backports.datetime_fromisoformat?
https://github.com/closeio/ciso8601/blob/master/why_ciso8601.md

My personal perspective is that backports.datetime_fromisoformat is a better use since it doesn't need an external library. But as I am new to the code base and still learning how it works, I wouldn't dare make this change by myself since it would need to change the rfc3339 format to use ISO format. I don't know if this would impact how the GoJuju library works, so I used the suggestion stated in the issue and used the ciso8601. But this is based in lack of experience with the library.

EdmilsonRodrigues · 2024-12-17T09:50:18Z

Re:
________________________ TestGoCookieJar.test_roundtrip ________________________

self = <tests.unit.test_gocookies.TestGoCookieJar testMethod=test_roundtrip>

def test_roundtrip(self):
    jar = self.load_jar(cookie_content)
    filename2 = os.path.join(self.dir, "cookies2")

  jar.save(filename=filename2)

tests/unit/test_gocookies.py:201:

juju/client/gocookies.py:46: in save
go_cookies.append(py_to_go_cookie(cookie))
juju/client/gocookies.py:107: in py_to_go_cookie
go_cookie["Expires"] = generate_rfc3339_from_unix_time(unix_time)

unix_time = datetime.datetime(2345, 11, 15, 18, 16, 8)

def generate_rfc3339_from_unix_time(unix_time: datetime.datetime) -> str:

  rfc, discard = unix_time.isoformat().split(".")

E ValueError: not enough values to unpack (expected 2, got 1)
Please run unit tests 🙇🏻
The easiest way is uvx tox -e unit

Thank you!! I didn't know yet how to run the unit tests. Fixing this.

james-garner-canonical · 2024-12-17T23:49:52Z

When #1101 was raised, I think we assumed that ciso8601 would be essentially a drop-in replacement. Thank you @EdmilsonRodrigues for taking a stab at this, and identifying that we not only need to go from rfc 3339 strings to datetimes, but in one case also from http.cookiejar.Cookie.expires (integer seconds) to a rfc 3339 string. I think it would be best if we didn't have to roll our own date time string generation here. Unfortunately as far as I can tell, ciso8601 strictly parses date time strings, and doesn't provide methods for generating them (anyone want to double check this?).

I'm +1 on using backports.datetime_fromisoformat instead of ciso8601 if it works for our use case, but this doesn't take care of generating rfc 3339 date time strings.

I haven't familiarised myself with the differences between the rfc 3339 and iso 8601 format @dimaqq, but is the output of datetime's isoformat compliant enough for us?

import datetime, time
timestamp = time.time()
timezone = datetime.timezone.utc
# or do we need local timezone?
# utcoffset = datetime.datetime.now().astimezone().utcoffset()  # can be None apparently
# timezone = datetime.timezone(utcoffset) if utcoffset is not None else datetime.timezone.utc
datetime.datetime.fromtimestamp(timestamp, tz=timezone).isoformat()
# '2024-12-17T23:34:38.780403+00:00'

dimaqq · 2024-12-18T07:26:23Z

@hpidcock commented on Matrix:

https://pkg.go.dev/time#Time.MarshalJSON
It just has to be rfc3339
https://github.com/juju/persistent-cookiejar is the library that was created for the juju client, if pylibjuju is to be able to open the cookie jar saved by the juju client, it just needs to be the same.

james-garner-canonical · 2024-12-18T22:45:43Z

Here is a useful but overwhelming reference to the differences and overlap between rfc 3339 and iso 8601.

I believe that this should be a perfectly valid rfc 3339 string, so I recommend we use datetime's isoformat for writing the string out:

datetime.datetime.fromtimestamp(timestamp, tz=timezone).isoformat()
# '2024-12-17T23:34:38.780403+00:00'

For parsing the date time strings we get from Juju, I suggest that we try backports.datetime_fromisoformat, and if it works in practice then we should just use it (and assume that juju/persistent-cookiejar won't be updated any time soon to use an esoteric form of rfc 3339 that's incompatible with iso 8601). However I'm not opposed to playing it safe and using ciso8601 if others would prefer that -- and if backports.datetime_fromisoformat doesn't work in our tests then we should just use ciso8601.

EdmilsonRodrigues · 2024-12-19T08:28:31Z

https://github.com/EdmilsonRodrigues/python-libjuju/blob/de772290a4582083fbd5f431efabada8374b12d4/tests/unit/test_gocookies.py#L206-L227

I don't know how to refer the part of the code the way you do, but I mean this:

    def test_expiry_time(self):
        content = """[
            {
                "CanonicalHost": "bar.com",
                "Creation": "2017-11-17T08:53:55.088820092Z",
                "Domain": "bar.com",
                "Expires": "2345-11-15T18:16:08Z",
                "HostOnly": true,
                "HttpOnly": false,
                "LastAccess": "2017-11-17T08:53:55.088822562Z",
                "Name": "bar",
                "Path": "/",
                "Persistent": true,
                "Secure": false,
                "Updated": "2017-11-17T08:53:55.088822562Z",
                "Value": "bar-value"
            }
        ]"""
        jar = self.load_jar(content)
        got_expires = tuple(jar)[0].expires
        want_expires = int(ciso8601.parse_rfc3339("2345-11-15T18:16:08Z").timestamp())
        self.assertEqual(got_expires, want_expires)

In this test, I think this is an example of a juju/persitent-cookiejar cookie.

This format is compatible with fromisoformat():

from datetime import datetime
datetime.fromisoformat("2345-11-15T18:16:08Z")
datetime.datetime(2345, 11, 15, 18, 16, 8, tzinfo=datetime.timezone.utc)
datetime.fromisoformat("2017-11-17T08:53:55.088822562Z")
datetime.datetime(2017, 11, 17, 8, 53, 55, 88822, tzinfo=datetime.timezone.utc)
So the parsing would work using fromisoformat.

I agree with using the backports.datetime_fromisoformat and test generating using the .isoformat().
I will wait for other opinions before doing anything, but I agree with the current point of view.

dimaqq · 2024-12-19T08:43:10Z

Would you consider making an alternative PR using backports?

EdmilsonRodrigues · 2024-12-19T08:45:11Z

Sure! I will open it in some minutes!

The tests are failing I'll fix this and open it tomorrow.

dimaqq · 2025-01-27T01:13:36Z

Closing in favour of #1247

#1247 …() and datetime.datetime.isoformat() #### Description *I tried using backports.datetime_fromisoformat but than the tox tests broke, probably because it required an import in the unit tests. I tried than just using the usual builtin datetime.datetime.fromisoformat and it worked just well.* *<Fixes: >* #### QA Steps *<Commands / tests / steps to run to verify that the change works:>* ``` tox -e py3 -- tests/unit/... ``` ``` tox -e integration -- tests/integration/... ``` All CI tests need to pass. *<Please note that most likely an additional test will be required by the reviewers for any change that's not a one liner to land.>* #### Notes & Discussion *This is a secondary branch from the Pull Request #1243 *

refactor: change use of pyrfc3339 to use ciso8601

8116a97

dimaqq requested review from dimaqq and james-garner-canonical December 17, 2024 06:35

dimaqq requested a review from Aflynn50 December 17, 2024 06:38

dimaqq reviewed Dec 17, 2024

View reviewed changes

chore: fix the malfunction so the tests pass

de77229

EdmilsonRodrigues mentioned this pull request Dec 19, 2024

chore: remove pyrfc3339 and change to datetime.datetime.fromisoformat… #1247

Merged

dimaqq closed this Jan 27, 2025

Conversation

EdmilsonRodrigues commented Dec 17, 2024

Description

QA Steps

Notes & Discussion

Uh oh!

dimaqq commented Dec 17, 2024

Uh oh!

dimaqq commented Dec 17, 2024

Uh oh!

dimaqq Dec 17, 2024

Choose a reason for hiding this comment

Uh oh!

EdmilsonRodrigues Dec 17, 2024

Choose a reason for hiding this comment

Uh oh!

dimaqq Dec 18, 2024

Choose a reason for hiding this comment

Uh oh!

dimaqq commented Dec 17, 2024

Uh oh!

dimaqq commented Dec 17, 2024

Uh oh!

dimaqq commented Dec 17, 2024

Uh oh!

EdmilsonRodrigues commented Dec 17, 2024

Uh oh!

EdmilsonRodrigues commented Dec 17, 2024

Uh oh!

james-garner-canonical commented Dec 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dimaqq commented Dec 18, 2024

Uh oh!

james-garner-canonical commented Dec 18, 2024

Uh oh!

EdmilsonRodrigues commented Dec 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dimaqq commented Dec 19, 2024

Uh oh!

EdmilsonRodrigues commented Dec 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dimaqq commented Jan 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

james-garner-canonical commented Dec 17, 2024 •

edited

Loading

EdmilsonRodrigues commented Dec 19, 2024 •

edited

Loading

EdmilsonRodrigues commented Dec 19, 2024 •

edited

Loading