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

Add Profile download button the Invocation page #10

Merged
merged 1 commit into from
Oct 7, 2024
Merged

Conversation

mortenmj
Copy link
Collaborator

@mortenmj mortenmj commented Oct 6, 2024

Screenshot 2024-10-06 at 22 26 41

If the user passes -browser-url, a download button is put in the top right corner when viewing an invocation. This links to bb-browser, allowing the profile to be downloaded.

TODO: It would be nice if the download button was scaled to the same size as the 'succeeded' and 'time' boxes/labels on the same row. Right now it is subtly larger.

@trey-ivy
Copy link
Collaborator

trey-ivy commented Oct 7, 2024

Wow you fixed CI as well! That is great! Overall this is a very nice addition to the project and thank you for putting this up. It does unfortunately completely break the invocation view on my machine. I did some debugging and the URL that I get for the profile is apparently formed differently than yours. Seems like you're expecting one less slash element in the string. As an example, here is what I see in the URI

bytestream://rbe.stackav.io/remote-execution/blobs/b180b5bf760640a36acbcb2a85d431ff050eb1c4d0b75af8978e9683d3b0294a/5374

And it causes the graphql query to return data that the application cannot map to a known type, so that breaks the view and all you see is an empty page with no content for the invocation. Here is what I saw in the graphql query when i was troubleshooting.

image

Also I have some feedback on where/how the buildbarn browser URL is provided. I dislike the idea of providing what is essentially frontend configuration information to the backend as a runtime flag. I think this should be supplied as an environment variable to the frontend (you can put it in the .env file). Then just build the URL on the fly on the frontend when you're displaying it to the user no need to prepend the broswerUrl to the item stored in the database.

Frankly, you shouldn't have to update the model or extend the schema in any way because this information is already stored in the relatedFiles object. Adding this profile object essentially just duplicates information already stored in the database. Below you can see an example of this.

image

I am however impressed that you understood how to update the schema and flow that information all the way through the frontend, and there could be a valid reason for this approach and needing to store this data in the backend, but I would prefer we just leverage the existing relatedFiles object either way.

After I updated the code so that it would work (think slice from the back) to grab the size in bytes and the hash correctly for my URL, I ran into an additional issue. My buildbarn browser service is deployed in a production environment and is secured using OIDC. When I actually tried to download the file, I got the following error:

rpc error: code = InvalidArgument desc = No valid OIDC session state cookie found

Now I'm okay if initially this doesn't work with auth as long as there is a backlog item to address this. I think if you explore the codebase you may find something already baked in to help with this as well

Copy link
Collaborator

@trey-ivy trey-ivy left a comment

Choose a reason for hiding this comment

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

  • please adjust so that differently formed URLs do not break the invocation view
  • please add a backlog item to make this work with OIDC for the browser service
  • please leverage the existing relatedFiles object

Copy link
Collaborator

@trey-ivy trey-ivy left a comment

Choose a reason for hiding this comment

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

Thanks for making those updates @mortenmj . it was unable to migrate my existing db and had to delete and recreate, but that is a small thing and i wouldn't block for it at this stage. There are some big changes coming to the db anyway for #11 Nice Work! Land it!

@mortenmj mortenmj merged commit d2ffcd4 into main Oct 7, 2024
1 check passed
@mortenmj mortenmj deleted the profile branch October 7, 2024 20:23
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.

2 participants