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

New input for Pixee API URL #15

Merged
merged 6 commits into from
May 16, 2024
Merged

New input for Pixee API URL #15

merged 6 commits into from
May 16, 2024

Conversation

carlosu7
Copy link
Contributor

No description provided.

@carlosu7 carlosu7 requested a review from gilday May 14, 2024 18:15
fjpgtt
fjpgtt previously requested changes May 14, 2024

const token = await core.getIDToken(AUDIENCE);
const token = await core.getIDToken(pixeeUrl);
Copy link

Choose a reason for hiding this comment

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

You need to keep the audience since that is https://app.pixee.ai not api.pixee.api

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I believe that's true, but it does beg the question, is https://app.pixee.ai always the right audience? Maybe it's technically incorrect in some deployments but still good enough? @ryandens please opine.

Copy link
Member

Choose a reason for hiding this comment

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

It captures the intended purpose (ensuring the person with permissions to generate this ID token generated it for Pixee). Changing the audience verification server side would be pretty trivial, but would require additional configuration when deploying a pixee server. I don't see a reason to not make this configurable now.

I also don't think we have a great reason for making the audience app.pixee.ai when the action uploads to api.pixee.ai - i think I picked the audience before we had the CNAME for this API. Open to pixee.ai or api.pixee.ai to match the target destination

Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason to configure them separately, or should we derive the audience from the URL?

Copy link

Choose a reason for hiding this comment

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

IMO we should derive the audience, that way one less input they need to pass. But I think this should be done in a different PR to avoid rejecting requests from the actions, since we need to update the server side validation.

Maybe we should first update the platform to check the audience to see if it is the old or the new api.pixee.ai value, after that change is deploy we can update the logic in the action to derive it from the URL. Finally, we can update the platform again to use something more configurable so it cover the pixee server case

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds good to me.

dist/index.js Outdated
Copy link

@fjpgtt fjpgtt May 14, 2024

Choose a reason for hiding this comment

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

You dont need to commit the changes of this file in the PR, the command will be executed when you deploy it

Copy link
Contributor

Choose a reason for hiding this comment

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

In fact, we should remove this file.

@carlosu7 carlosu7 requested a review from dhafley May 14, 2024 19:57
Copy link
Contributor

@gilday gilday left a comment

Choose a reason for hiding this comment

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

Let's make the audience change that @fjpgtt proposed.

@carlosu7 carlosu7 dismissed fjpgtt’s stale review May 15, 2024 22:55

re requesting it

@carlosu7 carlosu7 merged commit 993fda7 into main May 16, 2024
1 check passed
@carlosu7 carlosu7 deleted the host-input branch May 16, 2024 18:27
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.

4 participants