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.

Sign up for GitHub

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

Jump to bottom

Converge Import from Git/Devfile/Dockerfile flows #9832

Merged
merged 3 commits into from Aug 25, 2021
Merged

Converge Import from Git/Devfile/Dockerfile flows #9832

merged 3 commits into from Aug 25, 2021

Conversation

divyanshiGupta
Copy link
Contributor

@divyanshiGupta divyanshiGupta commented Aug 18, 2021 •
edited

@divyanshiGupta divyanshiGupta changed the title Converge Import from Git/Devfile/Dockerfile flows [WIP]Converge Import from Git/Devfile/Dockerfile flows Aug 18, 2021
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 18, 2021
@openshift-ci openshift-ci bot added component/dev-console Related to dev-console component/git-service Related to git-service labels Aug 18, 2021
@divyanshiGupta divyanshiGupta force-pushed the converged-import-flows branch 2 times, most recently from 7b0429c to 56e4b85 Compare August 19, 2021 20:58
@openshift-ci openshift-ci bot added the kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated label Aug 19, 2021
@divyanshiGupta divyanshiGupta force-pushed the converged-import-flows branch 2 times, most recently from ead37df to 333f23d Compare August 20, 2021 13:31
@divyanshiGupta divyanshiGupta changed the title [WIP]Converge Import from Git/Devfile/Dockerfile flows Converge Import from Git/Devfile/Dockerfile flows Aug 20, 2021
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 20, 2021
sahil143
sahil143 suggested changes Aug 20, 2021
View reviewed changes
frontend/packages/dev-console/src/components/import/ImportStrategySection.tsx Outdated Show resolved Hide resolved
frontend/packages/dev-console/src/components/import/ImportStrategySection.tsx Outdated Show resolved Hide resolved
frontend/packages/dev-console/src/components/import/ImportStrategySection.tsx Outdated Show resolved Hide resolved
frontend/packages/dev-console/src/components/import/ImportStrategySection.tsx Outdated Show resolved Hide resolved
frontend/packages/dev-console/src/components/import/ImportStrategySelector.tsx Outdated Show resolved Hide resolved
frontend/packages/dev-console/src/components/import/ImportStrategySelector.tsx Outdated Show resolved Hide resolved
frontend/packages/dev-console/src/components/import/ImportStrategySelector.tsx Outdated Show resolved Hide resolved
frontend/packages/dev-console/src/components/import/builder/BuilderImageTagSelector.tsx Outdated Show resolved Hide resolved
frontend/packages/dev-console/src/components/import/builder/BuilderSection.tsx Show resolved Hide resolved
frontend/packages/dev-console/src/components/import/devfile/DevfileInfo.tsx Outdated Show resolved Hide resolved
@sahil143
Copy link
Contributor

@divyanshiGupta found two issues

  1. clicking on the title of the recommended image seems to open the section that changes the strategy
  2. Clicking on other strategies doesn't seem to update the form

Kapture 2021-08-20 at 20 18 01

@rottencandy
Copy link
Contributor

If I enter a url and then change it, it still shows as validated:

0.mp4

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 20, 2021
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 21, 2021
@openshift-ci openshift-ci bot added the component/topology Related to topology label Aug 21, 2021
@divyanshiGupta divyanshiGupta force-pushed the converged-import-flows branch 2 times, most recently from 2086c1c to e16e3a2 Compare August 21, 2021 20:12
rohitkrai03
rohitkrai03 suggested changes Aug 22, 2021
View reviewed changes
frontend/packages/dev-console/src/components/edit-application/EditApplicationForm.tsx Show resolved Hide resolved
...tend/packages/dev-console/src/components/edit-application/__tests__/edit-application-data.ts Show resolved Hide resolved
frontend/packages/dev-console/src/components/import/ImportForm.tsx Show resolved Hide resolved
frontend/packages/dev-console/src/components/import/ImportStrategySection.tsx Outdated Show resolved Hide resolved
frontend/packages/dev-console/src/components/import/ImportStrategySelector.tsx Outdated Show resolved Hide resolved
frontend/packages/dev-console/src/components/import/builder/BuilderImageTagSelector.tsx Show resolved Hide resolved
frontend/packages/git-service/src/services/bitbucket-service.ts Show resolved Hide resolved
frontend/packages/git-service/src/services/bitbucket-service.ts Outdated Show resolved Hide resolved
@rohitkrai03
Copy link
Contributor

/assign

@rohitkrai03
Copy link
Contributor

@divyanshiGupta Tests are failing in git service.

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 23, 2021
@rohitkrai03
Copy link
Contributor

/label qe-approved

Verified all the below import flows on a cluster created using cluster bot -

  • Import git repo with builder image.
  • Import git repo with dockerfile.
  • Import git repo with devfile.
  • Import git repo with dockerfile but using builder image as import strategy.
  • Import builder image sample from sample catalog.
  • Import devfile sample from sample catalog.
  • Import builder image from developer catalog.
  • Import devfile from developer catalog.

Everything works as expected.

@openshift-ci openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Aug 23, 2021
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

1 similar comment
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@karthikjeeyar
Copy link
Contributor

/retest

1 similar comment
@rottencandy
Copy link
Contributor

/retest

@rohitkrai03
Copy link
Contributor

rohitkrai03 commented Aug 24, 2021 •
edited

/hold

Seems like this PR would need changes from #9835 as it changes e2e flows. @sanketpathak is working on rebasing his changes on top of this PR. We can get these changes merged together.

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 24, 2021
@openshift-ci openshift-ci bot added component/knative Related to knative-plugin component/pipelines Related to pipelines-plugin and removed lgtm Indicates that a PR is ready to be merged. labels Aug 24, 2021
@rohitkrai03
Copy link
Contributor

/lgtm
/hold cancel

Changes from #9835 and fixes for breaking e2e tests are now merged into this PR.

@openshift-ci openshift-ci bot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Aug 24, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 24, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: divyanshiGupta, invincibleJai, rohitkrai03

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

@sanketpathak sanketpathak mentioned this pull request Aug 24, 2021
Closed
5 tasks
@parvathyvr
Copy link

There are two Builder Image detected Alert box shown, is it intentional ? cc: @parvathyvr
image

I think this is a mistake.We do not want to display 2 inline information to the user.We should show only the 1st inline information above the 'Import strategy' section as shown here -The title should be‘Builder Image(s) detected’ and not show the 2nd one we used to have under the 'Builder Image' section.
As per our new design we have a hover behavior for the star icon as documented here
@divyanshiGupta

@rohitkrai03
Copy link
Contributor

/retest

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

2 similar comments
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@rohitkrai03
Copy link
Contributor

/hold

Merge queue is broken.

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 24, 2021
@rohitkrai03
Copy link
Contributor

/hold cancel
/retest

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 24, 2021
@openshift-merge-robot openshift-merge-robot merged commit d4d8055 into openshift:master Aug 25, 2021
@spadgett spadgett added this to the v4.9 milestone Aug 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@invincibleJai invincibleJai invincibleJai left review comments

@rohitkrai03 rohitkrai03 rohitkrai03 approved these changes

@andrewballantyne andrewballantyne Awaiting requested review from andrewballantyne

@jerolimov jerolimov Awaiting requested review from jerolimov

@rottencandy rottencandy Awaiting requested review from rottencandy

@sahil143 sahil143 Awaiting requested review from sahil143

Assignees

@rohitkrai03 rohitkrai03

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/dev-console Related to dev-console component/git-service Related to git-service component/knative Related to knative-plugin component/pipelines Related to pipelines-plugin component/topology Related to topology docs-approved Signifies that Docs has signed off on this PR kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated lgtm Indicates that a PR is ready to be merged. px-approved Signifies that Product Support has signed off on this PR qe-approved Signifies that QE has signed off on this PR
Projects
None yet
Milestone
v4.9
Development

Successfully merging this pull request may close these issues.

None yet