Skip to content
This repository has been archived by the owner on Jan 8, 2024. It is now read-only.

ui: Add Docker image/tag information and Overview section #2352

Merged
merged 6 commits into from
Oct 5, 2021

Conversation

sabrinako
Copy link
Contributor

@sabrinako sabrinako commented Sep 24, 2021

#1520

Original UI

Screen Shot 2021-09-30 at 5 24 20 PM

Updated UI

Screen Shot 2021-09-30 at 5 24 02 PM

How to Test

  1. Pull this branch down ui/docker-version
  2. Run a local Waypoint server (see here)
  3. Create a project
  4. waypoint up
  5. Check the deployment details page

@sabrinako sabrinako added ui pr/no-changelog No automatic changelog entry required for this pull request labels Sep 24, 2021
@sabrinako sabrinako added this to the 0.6.0 milestone Sep 24, 2021
@sabrinako sabrinako self-assigned this Sep 24, 2021
@sabrinako sabrinako removed the pr/no-changelog No automatic changelog entry required for this pull request label Sep 24, 2021
@koesbong koesbong linked an issue Sep 29, 2021 that may be closed by this pull request
@github-actions github-actions bot added the core label Sep 29, 2021
@sabrinako sabrinako changed the title ui: (wip) added component for docker image/tag ui: Add Docker image/tag information and Overview section Sep 30, 2021
@sabrinako sabrinako marked this pull request as ready for review September 30, 2021 22:12
@sabrinako sabrinako requested review from almonk and a team September 30, 2021 22:12
@jgwhite
Copy link
Contributor

jgwhite commented Oct 1, 2021

What this looks like when the status report is still pending:

CleanShot 2021-10-01 at 12 07 46@2x

internal/server/gen/server.pb.go Outdated Show resolved Hide resolved
return;
}

let container = this.args.statusReport.resourcesList.find((r) => r.type === 'container');
Copy link
Contributor

@jgwhite jgwhite Oct 1, 2021

Choose a reason for hiding this comment

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

I’m realizing that this method of digging up the image reference isn’t general enough. On the resource detail page (which I wrote) it only works for Kubernetes pods, and here it only works for Docker containers. We should collaborate on a general approach to this.

Copy link
Contributor

@jgwhite jgwhite Oct 1, 2021

Choose a reason for hiding this comment

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

I tried traversing everything in statusReport.resourcesList in search of image fields. This does get us what we want, but also some stuff we don’t want.

image

Here’s roughly what that approach would look like: cfbef23

ui/app/components/meta-table/index.hbs Outdated Show resolved Hide resolved
ui/app/components/meta-table/index.hbs Outdated Show resolved Hide resolved
ui/app/components/meta-table/index.ts Outdated Show resolved Hide resolved
ui/app/components/meta-table/index.ts Outdated Show resolved Hide resolved
@jgwhite
Copy link
Contributor

jgwhite commented Oct 1, 2021

By the way: I love how you’ve structured the “How to test” instructions. Going to steal that format going forward. Hope you don’t mind!

Copy link
Contributor

@gregone gregone left a comment

Choose a reason for hiding this comment

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

LGTM, just one minor comment

ui/app/components/container-image-tag/index.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@jgwhite jgwhite left a comment

Choose a reason for hiding this comment

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

Looking good. Just a few minor suggestions.

ui/app/components/container-image-tag/index.ts Outdated Show resolved Hide resolved
module('Integration | Component | meta-table', function (hooks) {
setupRenderingTest(hooks);

test('it renders', async function (assert) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I’d be tempted to split this test in two: one for the happy path and one for the empty state.

@@ -0,0 +1,29 @@
<div class="artifact-overview">
Copy link
Contributor

Choose a reason for hiding this comment

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

There are a few localizable strings in this file. Could we add them to en-us.yaml?

Copy link
Contributor

@jgwhite jgwhite left a comment

Choose a reason for hiding this comment

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

🥳

Sabrina Ko and others added 6 commits October 5, 2021 10:12
- added beginning HTMLElements for overview table component
- no tests
ui: added tests for MetaTable+ContainerImageTag

ui: added changelog

ui: fixed small bug

ui: account for duplicate image key

ui: fix failing tests from previous commit
Co-authored-by: Jamie White <jamie@jgwhite.co.uk>
much of the logic was written by @jgwhite in commit cfbef23

Co-authored-by: Jamie White <jamie@jgwhite.co.uk>
Co-authored-by: Jamie White <jamie@jgwhite.co.uk>
@sabrinako sabrinako merged commit c88aa4c into main Oct 5, 2021
@sabrinako sabrinako deleted the ui/docker-version branch October 5, 2021 14:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Display Docker Image Version
3 participants