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 pipeline url submit modal #2576

Conversation

Gkrumbach07
Copy link
Member

@Gkrumbach07 Gkrumbach07 commented Mar 8, 2024

closes: https://issues.redhat.com/browse/RHOAIENG-2299

Description

  • adds url upload option for both import pipeline and import pipeline version modal
    • uses radio button to switch between
  • adds two new pipelines api funcs to manage the import

image
image

How Has This Been Tested?

  1. open import pipeline modal
  2. fill out required fields
  3. select url import'
  4. use this for example https://raw.githubusercontent.com/amadhusu/data-science-pipelines/8dfd0c5fc8db1f4b18a8e22d13dacb4c02c50dbf/samples/core/exit_handler/exit_handler.py.yaml
  5. code source should be optional but url is not optional
  6. code source should not show up when doing file upload
  7. submit form
  8. pipeline should be in table
  9. do the same thing but for importing a pipeline version

Test Impact

added 2 tests for url import for pipeline and pipeline version

Request review criteria:

Self checklist (all need to be checked):

  • The developer has manually tested the changes and verified that the changes work
  • Commits have been squashed into descriptive, self-contained units of work (e.g. 'WIP' and 'Implements feedback' style messages have been removed)
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has added tests or explained why testing cannot be added (unit or cypress tests for related changes)

If you have UI changes:

  • Included any necessary screenshots or gifs if it was a UI change.
  • Included tags to the UX team if it was a UI/UX change (find relevant UX in the SMEs section).
    @yannnz @xianli123

After the PR is posted & before it merges:

  • The developer has tested their solution on a cluster by using the image produced by the PR to main

@openshift-merge-robot openshift-merge-robot added the needs-rebase PR needs to be rebased label Mar 8, 2024
@Gkrumbach07 Gkrumbach07 force-pushed the story/RHOAIENG-2299-improve-upload-modal branch from 3d261d8 to edf0e22 Compare March 8, 2024 13:38
@openshift-merge-robot openshift-merge-robot removed the needs-rebase PR needs to be rebased label Mar 8, 2024
Copy link
Contributor

@jpuzz0 jpuzz0 left a comment

Choose a reason for hiding this comment

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

I'm not entirely sure how Code source is meant to work for this feature. Entering in random characters when uploading with a URL seems to create a version and pipeline fine, but where is the Code source value used exactly in that process?

@Gkrumbach07
Copy link
Member Author

I'm not entirely sure how Code source is meant to work for this feature. Entering in random characters when uploading with a URL seems to create a version and pipeline fine, but where is the Code source value used exactly in that process?

It will be used later on in details. As of now it does nothing.

@Gkrumbach07 Gkrumbach07 force-pushed the story/RHOAIENG-2299-improve-upload-modal branch 2 times, most recently from 305c8a5 to 717c120 Compare March 11, 2024 21:31
Copy link
Contributor

openshift-ci bot commented Mar 11, 2024

@Gkrumbach07: you cannot LGTM your own PR.

In response to this:

This looks good to me. so ill give it the lgtm but @xianli123 Is this good to go fro a UX perspective? Can we follow up on any other UX changes?

Right now it looks like every field is required (strings, bools, structs, lists, ...), and there are no prefilled values, so a user must give a value for everything, even strings.

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@Gkrumbach07 Gkrumbach07 changed the base branch from f/pipelines-v2 to main March 12, 2024 12:53
@jpuzz0
Copy link
Contributor

jpuzz0 commented Mar 12, 2024

I'm not entirely sure how Code source is meant to work for this feature. Entering in random characters when uploading with a URL seems to create a version and pipeline fine, but where is the Code source value used exactly in that process?

It will be used later on in details. As of now it does nothing.

If this does nothing right now, should we not change the scope of the ticket? The JIRA states:
Support source code upload as acceptance criteria.

@Gkrumbach07
Copy link
Member Author

@jpuzz0

If this does nothing right now, should we not change the scope of the ticket? The JIRA states:
Support source code upload as acceptance criteria.

I think source code upload was a bit of an unknown option and was added becuase it was in KF. This fields does submit source code now. it places a valid field in the pipeline version

@jpuzz0
Copy link
Contributor

jpuzz0 commented Mar 12, 2024

/lgtm

@xianli123
Copy link

Thanks @Gkrumbach07. From the UX point of view, it looks good. One suggestion is that the Description field is usually a text area.

@yannnz
Copy link

yannnz commented Mar 14, 2024

@jpuzz0

If this does nothing right now, should we not change the scope of the ticket? The JIRA states:
Support source code upload as acceptance criteria.

I think source code upload was a bit of an unknown option and was added becuase it was in KF. This fields does submit source code now. it places a valid field in the pipeline version

This screenshot below shows the code source "information displayed under the pipeline Summary tab. It is for the version source link. I put a note on the image based on what Humair explained. Not sure if it is helpful though.
Screenshot 2024-03-14 at 13 30 15

So for those two screenshots in this PR, except what Kun mentioned above about the description field, the Upload a file option also always shows the source code input box in KBF UI.

@yannnz
Copy link

yannnz commented Mar 14, 2024

I'm not entirely sure how Code source is meant to work for this feature. Entering in random characters when uploading with a URL seems to create a version and pipeline fine, but where is the Code source value used exactly in that process?

It will be used later on in details. As of now it does nothing.

If this does nothing right now, should we not change the scope of the ticket? The JIRA states: Support source code upload as acceptance criteria.

Is it saying that we will add the code source field later in another ticket?

BTW, I added the slack thread here for the source code discussion if you need it: https://redhat-internal.slack.com/archives/C05U1Q749PV/p1706101044229369

@Gkrumbach07 Gkrumbach07 force-pushed the story/RHOAIENG-2299-improve-upload-modal branch from 717c120 to 6f45795 Compare March 14, 2024 14:22
@openshift-ci openshift-ci bot removed the lgtm label Mar 14, 2024
Copy link
Contributor

openshift-ci bot commented Mar 14, 2024

New changes are detected. LGTM label has been removed.

@jpuzz0
Copy link
Contributor

jpuzz0 commented Mar 14, 2024

Checked this out locally again and re-tested. Looks good still along with recent code changes. Once tests pass, this should be good to go.

@Gkrumbach07 Gkrumbach07 force-pushed the story/RHOAIENG-2299-improve-upload-modal branch from 6f45795 to d2cb89e Compare March 14, 2024 18:23
@Gkrumbach07 Gkrumbach07 force-pushed the story/RHOAIENG-2299-improve-upload-modal branch 2 times, most recently from 5151948 to 9bd964a Compare March 14, 2024 19:02
Add code source URL to pipeline import modal

Refactor pipeline import and version import modals

Refactor PipelineUploadRadio component

Remove code source input from PipelineImportModal and PipelineVersionImportModal

fix test

Remove unused imports and descriptions in Pipelines.cy.ts

Add pipeline version description

Update pipeline version description
@Gkrumbach07 Gkrumbach07 force-pushed the story/RHOAIENG-2299-improve-upload-modal branch from 9bd964a to d127a4b Compare March 14, 2024 19:20
Copy link
Contributor

openshift-ci bot commented Mar 14, 2024

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: jpuzz0

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot openshift-merge-bot bot merged commit 9667154 into opendatahub-io:main Mar 14, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants