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 container source form in event source add flow #5061

Merged

Conversation

karthikjeeyar
Copy link
Contributor

@karthikjeeyar karthikjeeyar commented Apr 15, 2020

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

Analysis / Root cause:
Container source option is missing in event source creation flow.

Solution Description:
Added container source option in event source creation flow
doc: https://github.com/knative/docs/tree/master/docs/eventing/samples/container-source

screenshot:

container-source

Form structure:
image

Unit test converage:
containerSourceSection:
image
containersource validations:
image

Browser conformance:

  • Chrome
  • Firefox
  • Safari
  • Edge

cc: @serenamarie125 @christianvogt

@openshift-ci-robot openshift-ci-robot added the component/knative Related to knative-plugin label Apr 15, 2020
@karthikjeeyar
Copy link
Contributor Author

/kind feature
/assign @invincibleJai

@openshift-ci-robot openshift-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Apr 15, 2020
Copy link
Contributor

@serenamarie125 serenamarie125 left a comment

Choose a reason for hiding this comment

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

@karthikjeeyar this is looking good, thanks for doing this! Approved by UX

We will likely have some copy updates for help text, but UX won't be proposing til all PRs for Feature Freeze are merged.

@karthikjeeyar
Copy link
Contributor Author

karthikjeeyar commented Apr 16, 2020

@karthikjeeyar this is looking good, thanks for doing this! Approved by UX

We will likely have some copy updates for help text, but UX won't be proposing til all PRs for Feature Freeze are merged.

Thanks Serena, we will have tech debt item to align with UX updates post FF.

Comment on lines +157 to +164
containers: [
{
image: '',
name: '',
args: [{ name: '' }],
env: [],
},
],
Copy link
Member

Choose a reason for hiding this comment

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

can we have

template: {
    spec: {
      containers: [
        {
          image: '',
          name: '',
          args: [{ name: '' }],
          env: [],
        },
      ]
  }
}

then we can pass it as is to getEventSourcesDepResource and avoid util getContainerSourceResource

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We cannot avoid the util function as MulticolumnField value return an array objects
ie: args: [ {name: 'x'} ] which is needs transformation in that util function to make it array of strings. eg: `args: ['x'].

@karthikjeeyar karthikjeeyar force-pushed the container-source-form branch 2 times, most recently from bfe0748 to dae951f Compare April 16, 2020 08:18
@invincibleJai
Copy link
Member

Thanks @karthikjeeyar Verified locally , works as expected

/approve

/assign @sahil143

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 16, 2020
import { getSuggestedName } from '@console/dev-console/src/utils/imagestream-utils';

const ContainerSourceSection: React.FC = () => {
const envPath = 'data.containersource.containers[0].env';
Copy link
Contributor

Choose a reason for hiding this comment

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

If there is a case one of the values prop is undefined then please use optional chaining to get env values in a const something like envValues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I use this envPath in setFieldValue function below, so it will not work if i use optional chaining path in Formik and moreover the values has a default env values as initialValues which will be always present.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on your previous comment, if we are destructuring then we no longer need this Path variable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes agreed! After destructuring, this is not needed. this was just a suggestion in case some values are optional.

const envPath = 'data.containersource.containers[0].env';
const { values, setFieldValue } = useFormikContext<FormikValues>();

const initialEnvValues = !_.isEmpty(values[envPath])
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 initialEnvValues = !_.isEmpty(values[envPath])
const initialEnvValues = !_.isEmpty(envValues)

const { values, setFieldValue } = useFormikContext<FormikValues>();

const initialEnvValues = !_.isEmpty(values[envPath])
? _.map(values[envPath], (env) => _.values(env))
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

then: yup.object().shape({
containersource: yup.object().shape({
containers: yup.array().of(
yup.object({
Copy link
Contributor

Choose a reason for hiding this comment

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

I think here also it should be yup.object().shape({image: yup.string().required('Required')})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can either use yup.object().shape({x: y}) or directly pass the shape in the object's constructor yup.object({x:y}). Both does exactly the same thing.
Refer: https://github.com/jquense/yup#object
Usually, i do object().shape if there is nested object, for plain one level object i pass the shape inside the object(shape)

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay. Got it. Thanks

@divyanshiGupta
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 16, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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-robot openshift-merge-robot merged commit de83364 into openshift:master Apr 16, 2020
@spadgett spadgett added this to the v4.5 milestone Apr 20, 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. component/knative Related to knative-plugin kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants