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

Bug 1807868: fixes issue with knative service in edit flow #4528

Merged

Conversation

invincibleJai
Copy link
Member

Fixes:
https://issues.redhat.com/browse/ODC-3069

Analysis / Root cause:
On editing knative Service from topology view with different image not creating new revision.

Solution Description:
In case of knative service edit generating new Image Stream with name-randomChars and update service with new image registry url and filtering image stream based on instance name.

Screen shots / Gifs for design review:
ezgif com-optimize (1)

Browser conformance:

  • Chrome
  • Firefox
  • Safari
  • Edge

@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. component/dev-console Related to dev-console labels Feb 27, 2020
@invincibleJai invincibleJai changed the title fixes issue with knative service in edit flow Bug 1807868: fixes issue with knative service in edit flow Feb 27, 2020
@openshift-ci-robot
Copy link
Contributor

@invincibleJai: This pull request references Bugzilla bug 1807868, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.5.0) matches configured target release for branch (4.5.0)
  • bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1807868: fixes issue with knative service in edit flow

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.

@openshift-ci-robot openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Feb 27, 2020
@@ -5,7 +5,7 @@ export interface AppResources {
service?: FirehoseResult<K8sResourceKind>;
route?: FirehoseResult<K8sResourceKind>;
buildConfig?: FirehoseResult<K8sResourceKind>;
imageStream?: FirehoseResult<K8sResourceKind>;
imageStream?: FirehoseResult;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
imageStream?: FirehoseResult;
imageStream?: FirehoseResult<K8sResourceKind[]>;

Copy link
Member

@vikram-raj vikram-raj left a comment

Choose a reason for hiding this comment

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

Tested all the scenario it working fine with this changes.
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 27, 2020
@@ -22,6 +22,7 @@ import {
import { RegistryType } from '../../utils/imagestream-utils';
import { AppResources } from '../edit-application/edit-application-types';
import { DeployImageFormData, Resources } from './import-types';
import { getRandomChars } from '../pipelines/pipeline-resource/pipelineResource-utils';
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to be putting this function in a nicer location... import for knative pulling from pipelines is odd.

We can probably leave this for the sake for 4.4 getting done but we should log a refactor task to clean this up.

We could probably keep this to dev-console though...

Copy link
Member Author

Choose a reason for hiding this comment

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

can we have it in under dev-console/utils in shared-submit-utils.ts ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that's works

@@ -90,12 +95,18 @@ export const createOrUpdateImageStream = (
],
},
};

if (formData.resources === Resources.KnativeService && !_.isEmpty(originalImageStream)) {
const OrignalImageStreamTag = _.find(originalImageStream?.status?.tags, ['tag', tag]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const OrignalImageStreamTag = _.find(originalImageStream?.status?.tags, ['tag', tag]);
const orignalImageStreamTag = _.find(originalImageStream?.status?.tags, ['tag', tag]);

Not sure why this got a capital... it's just a value like anything else.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated


let generatedImageStreamName: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh yikes... the reason this is outside of the function is that you're sharing it between functions... this is not ideal in anyway. Bump this variable up a scope and then pass it through to the functions as needed.

Looks like the goal of this is to revalue name... perhaps do it seamlessly at the top level by reworking the "formData" to include the repurposed name before it's passed down. This feels like a hack where one (I don't think) is needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, this was bad. have updated it. Thanks

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 2, 2020
@invincibleJai invincibleJai force-pushed the fix-kn-svc-img-edit branch 2 times, most recently from b21fe93 to 71fb7d2 Compare March 2, 2020 12:46
@andrewballantyne
Copy link
Contributor

/kind bug

@openshift-ci-robot openshift-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Mar 2, 2020
Copy link
Contributor

@andrewballantyne andrewballantyne left a comment

Choose a reason for hiding this comment

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

Tiny typo

@@ -402,11 +401,22 @@ export const createOrUpdateDeployImageResources = async (
// Do not run serverless call during the dry run.
let imageStreamUrl: string = image?.dockerImageReference;
if (registry === RegistryType.External) {
let generatedImageStreamName: string = '';
if (imageStreamList && imageStreamList.length && verb === 'update') {
const orignalImageStreamTag = _.find(originalImageStream?.status?.tags, [
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const orignalImageStreamTag = _.find(originalImageStream?.status?.tags, [
const originalImageStreamTag = _.find(originalImageStream?.status?.tags, [

Copy link
Contributor

@andrewballantyne andrewballantyne left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 2, 2020
@invincibleJai
Copy link
Member Author

/cherry-pick release-4.4

@openshift-cherrypick-robot

@invincibleJai: once the present PR merges, I will cherry-pick it on top of release-4.4 in a new PR and assign it to you.

In response to this:

/cherry-pick release-4.4

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.

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, invincibleJai, vikram-raj

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-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 2, 2020
@andrewballantyne
Copy link
Contributor

/test analyze

Bundle size flake.

@andrewballantyne
Copy link
Contributor

✖ should deploy the image and display it in the topology (3 failures)

Well that's not a good test to see fail... Going to rerun... hopefully this is not one of those times this flake holds down the PR.

/retest

@openshift-bot
Copy link
Contributor

/retest

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

2 similar comments
@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@christianvogt
Copy link
Contributor

/cherrypick release-4.4

@openshift-cherrypick-robot

@christianvogt: once the present PR merges, I will cherry-pick it on top of release-4.4 in a new PR and assign it to you.

In response to this:

/cherrypick release-4.4

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.

@openshift-bot
Copy link
Contributor

/retest

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

3 similar comments
@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@invincibleJai
Copy link
Member Author

invincibleJai commented Mar 3, 2020

will force push help here?

out of 13 runs , twice pull-ci-openshift-console-master-e2e-gcp-console passed looks like

Screenshot 2020-03-03 at 8 45 26 AM

@openshift-bot
Copy link
Contributor

/retest

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

8 similar comments
@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-merge-robot openshift-merge-robot merged commit b4ea75c into openshift:master Mar 3, 2020
@openshift-ci-robot
Copy link
Contributor

@invincibleJai: All pull requests linked via external trackers have merged. Bugzilla bug 1807868 has been moved to the MODIFIED state.

In response to this:

Bug 1807868: fixes issue with knative service in edit flow

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.

@openshift-cherrypick-robot

@invincibleJai: new pull request created: #4610

In response to this:

/cherry-pick release-4.4

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.

@spadgett spadgett added this to the v4.5 milestone Mar 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. component/dev-console Related to dev-console kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants