Add upload image functionality and a create image upload wizard

[jkozol]

The create image modal has been replaced with a wizard that can create an image while also allowing the user to upload the image to a select group of cloud providers. This wizard guides the user through selecting the image type and where to upload the image, then has the user enter the settings needed to upload the image, and finally has the user review the upload settings before uploading the image. The redux store and sagas have also been updated to handle the upload data.

This PR is dependent on weldr/lorax#843

This PR includes only a basic uploads list attached to each image in the images list and no ability to create an upload for an already created image. There are also a few of console warnings which still need to be fixed:

• Each child in a list should have a unique "key" prop error caused by the mapping of TextListItems

• Changing an uncontrolled input of type text to be controlled caused by the use of TextInputs

• Inline style errors caused by Patternfly 4 (fixed in the recent version of patternfly which requires additional configuration in our webpack config)

[jgiardino]

🎉 🎉 🎉 This is awesome!

There are some console errors that prevented me from accessing some of the updates, but here are some comments and questions on what I could review...

• Should we provide a default value for image names? But also, is the user naming the file? Or what are they naming? And this question might be out of scope of this PR, but when we add support for listing the compose/manifest/osbuild from which an image file is created, would we name that for the user or let them provide a name? Maybe we can chat about all of this, and maybe at standup to include those working on osbuild.
• We can add styles to fix the issue with the alignment of checkboxes in the form, and also the static text at the top:

  .pf-c-form__horizontal-group .pf-c-check,
  .cc-c-form__static-text {
  padding-top: var(--pf-global--spacer--form-element);
    padding-bottom: var(--pf-global--spacer--form-element);
  }
  
  .pf-c-form__horizontal-group .pf-c-check__label {
    line-height: var(--pf-global--LineHeight--md);
  }
  

  • The class cc-c-form__static-text would be added to the <p> element that includes the blueprint name. I was thinking we could start namespacing all of the classes we add to PF4 components with cc- for "cockpit-composer".
• We should also use slightly different html to make the checkbox form group more accessible:

  <div class="pf-c-form__group">
    <span class="pf-c-form__label" id="horizontal-form-checkbox">
      <span class="pf-c-form__label-text">Upload Image</span>
    </span>
    <fieldset class="pf-c-form__horizontal-group" aria-labelledby="horizontal-form-checkbox">
      <div class="pf-c-check">
        <input id="azure-checkbox" class="pf-c-check__input" type="checkbox" aria-invalid="false" value="azure">
        <label class="pf-c-check__label" for="azure-checkbox">Upload to Azure</label>
      </div>
    </fieldset>
  </div>
  

  • The <label> that displays on the left become a <span> with an id.
  • The div.pf-c-form__horizontal-group becomes a <fieldset> element, and instead of a <legend> to label this fieldset, there's an aria-labelledby attribute that references the id of the label on the left.

[jkozol]

Should we provide a default value for image names? But also, is the user naming the file? Or what are they naming?

Okay so asking for Image Name may actually be misleading. The user is setting the image name for the upload. For example, if the user uploads an image to a service with the name "database-image" they will see an image with that name in their cloud provider. But, the image that we show in the UI will no have that user defined name. Only the uploads for a given image will have user defined names. Do you think we should change the terminology?

And this question might be out of scope of this PR, but when we add support for listing the compose/manifest/osbuild from which an image file is created, would we name that for the user or let them provide a name? Maybe we can chat about all of this, and maybe at standup to include those working on

I agree that this is a good question to be be discussed in more detail at either the standup or engineering meeting. In my opinion I think we should allow users to name their compose, then have the image be named from the compose and image type such as "compose-name-tar", and finally allow the user to specify the name for their upload.

I will update the styles per your recommendations.

[jgiardino]

After pulling the latest and rebuilding again, I'm only getting console errors when I try to navigate to the blueprint page.

[KKoukiou]

Some very abstract initial feedback.

[KKoukiou]

The modal title is 'Create and Upload Image', however for many image types upload operation is not supported.

[jkozol]

Do you think we should start the title as Create Image and then if the user selects an upload provider change it to Create and Upload Image?

[jgiardino]

I missed this in previous reviews, but PF4 recommends sentence-style capitalization for all text strings (except proper nouns and product names). We should start adopting the PF4 style guide for all new UI or UI updates.

[jkozol]

@jgiardino I believe all of the capitalization should be fixed now by the Fix capitalization in CreateImageUpload Wizard commit. I will squish this commit after your review.

[jkozol]

This depends on #776 now which is included in this PR as well.

[codecov-io]

Codecov Report

❗ No coverage uploaded for pull request base (master@c86541c). Click here to learn what that means.
The diff coverage is 60.18%.

@@            Coverage Diff            @@
##             master     #767   +/-   ##
=========================================
  Coverage          ?   77.55%           
=========================================
  Files             ?       72           
  Lines             ?     3489           
  Branches          ?      738           
=========================================
  Hits              ?     2706           
  Misses            ?      783           
  Partials          ?        0

Impacted Files	Coverage Δ
main.js	88% <ø> (ø)
core/reducers/index.js	100% <ø> (ø)
pages/blueprintEdit/index.js	74.14% <ø> (ø)
core/store.js	100% <ø> (ø)
pages/blueprint/index.js	68.64% <ø> (ø)
components/ListView/BlueprintListView.js	100% <ø> (ø)
components/ListView/ListItemUploads.js	10% <10%> (ø)
core/actions/composes.js	100% <100%> (ø)
core/sagas/composes.js	68.65% <100%> (ø)
core/reducers/uploads.js	100% <100%> (ø)
... and 7 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c86541c...aab09c0. Read the comment docs.

[jgiardino]

There is an issue on the Edit page with the current css defined for the toolbars on that page and the new PF4 wizard. I noticed that if I changed z-index: 1000; to z-index: 1; for this ruleset, then the issue is fixed.

[jgiardino]

I'm still waiting for the image build process to complete, but here are some suggestions and questions about what I was able to do today...

• Is it possible to complete the authentication step when the user clicks Next? I think this could be captured as a separate issue, but it's worth noting that if the authentication failed during the upload process, then the user would have to download the file and upload it in the portal (or some other manual method) since we don't have the ability to upload images that were previously built.
• We should provide embedded help with information on where to find this information (this can be a separate PR: upload image - embedded help for Azure images #781)
• Authentication step
  • We should use similar text strings that are used in the portal, and put them in this order so that it matches the order that users will discover them in the portal:
    • Subscription ID
    • Application (client) ID
    • Directory (tenant) ID
    • Secret
    • Resource group
    • Storage account
  • Are resource group and storage account necessary information for authenticating?
• File upload step
  • I’m not able to type anything into the Image name field
  • Is location necessary? E.g. when I look at the cli for this action, i’m not seeing location as a parameter that can be provided, and I'm not seeing it in the Portal UI for uploading a blog either.
  • Maybe this is a better label to use, which combines what’s used in the cli vs the portal. It looks like the cli might be using original terminology "storage container" whereas "Blob" seems to have become the more prominent term in the portal UI?
    • Storage (blob) container

[bcl]

* Is it possible to complete the authentication step when the user clicks Next? I think this _could_ be captured as a separate issue, but it's worth noting that if the authentication failed during the upload process, then the user would have to download the file and upload it in the portal (or some other manual method) since we don't have the ability to upload images that were previously built.

Because of how the playbooks work the authentication is part of the process of executing the upload, there is no separate authentication step.

BUT there is good news -- we can upload previously built images. That's what the /api/v1/compose/uploads/schedule/<build_id> route is for (I'm actually using this, repeatedly, in my attempts to get an AWS playbook workgin).

* We should provide embedded help with information on where to find this information (this can be a separate PR: #781)

As I commented in the other PR, seems like this should be added to the provider.toml that drives the values used for the playbook. /api/v1/upload/providers lists all the required values, their defaults, validation regex, etc. help could be added there.

[jkozol]

Currently, this PR only contains support for uploading to AWS since it is the only service I have successfully tested. Once I am able to test vsphere and openstack I will either add those to this PR or open another one.

[jgiardino]

I tested the AWS flow. I don't think I was able to upload anything, unless it's taking a really long time. I'm assuming it failed, but I'm not sure what the point of failure was. Ultimately, this is something we should address (i.e. either eliminate or expose points of failure in the UI), but I'm not sure we can in the scope of this PR.

Aside from that, here are some suggestions:

• I would use “Create image” instead of “Create and upload image” as the wizard title since image creation is the primary task. Image uploads are secondary and optional, and not available with all types
• Upload to AWS field labels -
  • These should be translated, except for “bucket” which appears to be the only string not translated in AWS.
  • Here are some suggestions on alternate labels
    • AWS access key >>> AWS access key ID
    • AWS secret key >>> AWS secret access key
    • AWS bucket >>> Amazon S3 bucket name
      • In the AWS console, I see strings like “Amazon S3”, “S3 buckets” and “bucket name”.
    • I think Image name is fine, until we get further feedback.
  • On the review screen, the order of fields is different. Was that intentional? I'd suggest keeping them in the same order they're presented in the previous screens.
  • I’m not able to enter any text in the Image name field.
  • Are all fields required? We should include a statement about required fields. Based on the PF design documentation for indicating required fields, if they're all required, we can include a statement above the form saying so. In this case, we would still want isRequired on the <TextInput> but would remove it from <FormGroup> to just remove the red asterisk. Otherwise, if some are required and some are optional, we can use similar patterns that we have in other forms (like Create Blueprint)
    • How do you want to handle missing required fields? I’m thinking on the review screen we can block them from clicking Finish if any required fields are missing (and in the future we can extend this to also block if authentication is failing).
  • Is the AWS region required? I’m mostly confused about this field.
    • Do you happen to know what it’s purpose is? When I review the documentation on weldr.io, there’s no mention of specifying a region when you create a bucket or uploading to a bucket. And if the bucket is totally unique, I’m curious why we need to ask for it here.
    • If region is important to capture here, then do I enter the string as it’s displayed in the AWS console? Or is it possible to supply this as a set of options? (maybe as a future enhancement)

[jkozol]

@jgiardino Thank you for the detailed review.

I would use “Create image” instead of “Create and upload image” as the wizard title since image creation is the primary task. Image uploads are secondary and optional, and not available with all types

That makes sense. I've updated the string.

These should be translated, except for “bucket” which appears to be the only string not translated in AWS

Still working on fixing this.

AWS access key >>> AWS access key ID
AWS secret key >>> AWS secret access key
AWS bucket >>> Amazon S3 bucket name

All switched

On the review screen, the order of fields is different. Was that intentional? I'd suggest keeping them in the same order they're presented in the previous screens.

The image name field has been moved into the correct position.

Are all fields required? We should include a statement about required fields. Based on the PF design documentation for indicating required fields, if they're all required, we can include a statement above the form saying so. In this case, we would still want isRequired on the but would remove it from to just remove the red asterisk. Otherwise, if some are required and some are optional, we can use similar patterns that we have in other forms (like Create Blueprint)

I have updated this per the suggestions in the documentation

How do you want to handle missing required fields? I’m thinking on the review screen we can block them from clicking Finish if any required fields are missing (and in the future we can extend this to also block if authentication is failing).

Do you happen to know what it’s purpose is? When I review the documentation on weldr.io, there’s no mention of specifying a region when you create a bucket or uploading to a bucket. And if the bucket is totally unique, I’m curious why we need to ask for it here.

The region is used to create the bucket.

If region is important to capture here, then do I enter the string as it’s displayed in the AWS console? Or is it possible to supply this as a set of options? (maybe as a future enhancement)

Yes. You should enter the region as listed here. This is good idea. Having a set of regions for users to select from would be useful.

[larskarlitski]

Rebased on master to include #836.

[martinpitt]

Sorry, this needs rebasing after #833, i. e. the tests need to be adjusted to the newer (and simpler) webdriver v5 style.

[jgiardino]

I'm testing this out and am having trouble getting a successful image build, but there are a couple of things I have noticed. These might not be related to this PR specifically, but wanted to note them here, since I'm currently looking at this PR. Let me know if you think these are actually separate issues.

In the example below showing RHEL:

• Both of these images have uploads to AWS, but the image type is displaying as Alibaba.

In the example below showing Fedora 32:

• The item with status "Image build in progress" seems like a bug. It has an older timestamp, and the Logs just say Running.... I really have no idea how to reproduce this issue though.
• When I originally implemented this updated list view for images and uploads, I think I was expecting all images to have an uploads array when uploads were supported. I included the uploads toggle at a time when uploads weren't supported, and used this line to not show the uploads toggle if there are no uploads defined. But, assuming all images would have an uploads array, even if empty, I implemented this so that the toggle would be included but with a class to visually hide it if no uploads. This is needed to create alignment within the list (e.g. the status messages should align whether uploads exist or not). This can definitely be a separate issue, since it's not critical, but wanted to mention it in case it's a super easy fix.

[ondrejbudai]

Both of these images have uploads to AWS, but the image type is displaying as Alibaba

This is a known issue on the osbuild-composer side: osbuild/osbuild-composer#369

[ondrejbudai]

I looked into the upload issues, afaik everything works correctly in cockpit-composer. However, I found this issue in osbuild-composer:

Uploads to us-east-1 are failing, because this region seems to be currently very slow and the snapshot import part of the upload process is timing out. I created this PR to fix it: osbuild/osbuild-composer#500.

@jgiardino

[jgiardino]

Thanks for the update, @ondrejbudai!

I have tested this UI in both the RHEL and F32 auto-deploy instances, and was successfully able to build and upload an image. 🎉

[jgiardino]

Testing the UI, I'm able to successfully build and upload images. Additional refinements can be handled with separate issues/PRs.

Thanks for working on this!!