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

Use authlib #662 branch to fix token introspection endpoint (#219) #223

Merged
merged 1 commit into from
Aug 1, 2024

Conversation

AdamWill
Copy link
Contributor

No description provided.

@hluk
Copy link
Member

hluk commented Jul 19, 2024

requirements.txt contain dependencies only for the builder image. Dependencies for the final image are in pyproject.toml (you would also need to update the lock file afterwards with poetry update --lock).

It is interesting idea, but I'm not sure how much I like fixing a dependency to some git commit. We could miss some security updates.

Another alternative is to replace authlib in the BuildConfig (I assume this is still used) - though this final image won't be tested properly before deploying.

RUN /venv/bin/pip install \
        "authlib @ git+https://github.com/AdamWill/authlib@oauth2-fix-introspect-endpoint"

@AdamWill
Copy link
Contributor Author

Changed to use poetry. Ugh, I hate poetry.

@AdamWill
Copy link
Contributor Author

btw, the requirements.txt version did work, can't tell you how, but it did. I tested it.

This is intended to only be temporary until the PR is merged. Upstream seems to move slow. Of course I can update the branch with anything else that is merged upstream, but I'd have to remember to do that, or we can use a branch you guys own and you can update it.

@AdamWill
Copy link
Contributor Author

I'm not sure whether that buildconfig is ever still used. In the imagestream definition we have both 'waiverdb' and 'waiverdb-upstream' entries. I think rolling out 'waiverdb' would roll out the image built by that buildconfig, rolling out 'waiverdb-upstream' rolls out the upstream image directly (usually from your quay.io repo, but currently I have staging set to use mine so I can mess with it). I've been rolling out waiverdb-upstream. I'm not actually sure what gets rolled out "by default".

@AdamWill
Copy link
Contributor Author

AdamWill commented Jul 19, 2024

Actually I'm running oc -n waiverdb rollout latest waiverdb-web. It's possible waiverdb is getting rebuilt and that's what I'm rolling out, I really don't know...I guess that would explain why there's always a lag between me pushing a new image to quay.io and actually being able to roll it out...

@mvalik
Copy link
Contributor

mvalik commented Jul 19, 2024

I like this solution: quite small and elegant. Maybe add some TODO comment to change it back when the new authlib is released.

@AdamWill
Copy link
Contributor Author

AdamWill commented Jul 19, 2024

the comment was already kinda meant to imply that, but I guess I can make it more explicit indeed. edit: oh, oops, and the comment got completely lost in moving from requirements.txt to pyproject.toml! putting it back...

@AdamWill
Copy link
Contributor Author

We've had this deployed in Fedora prod and staging for a while now (by deploying from my container remote instead of the official one). It's definitely working, I see that folks are filing waivers via Bodhi. Can we merge this and get the official container remote updated? Thanks!

Copy link
Member

@hluk hluk left a comment

Choose a reason for hiding this comment

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

One minor comment, otherwise good to merge! 👍

Dockerfile Outdated Show resolved Hide resolved
@hluk hluk merged commit 583c62f into release-engineering:master Aug 1, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants