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

feat: UI enhancement for Cluster Workflow Template #2525

Merged
merged 153 commits into from
Apr 4, 2020

Conversation

sarabala1979
Copy link
Member

@sarabala1979 sarabala1979 commented Mar 26, 2020

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
  • The title of the PR is (a) conventional, (b) states what changed, and (c) suffixes the related issues number. E.g. "fix(controller): Updates such and such. Fixes #1234". feat: UI enhancement for Cluster Workflow Template #2525
  • I have written unit and/or e2e tests for my change. PRs without these are unlike to be merged.
  • Optional. I've added My organization is added to the USERS.md.
  • I've signed the CLA and required builds are green.

@codecov
Copy link

codecov bot commented Apr 2, 2020

Codecov Report

Merging #2525 into master will not change coverage by %.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2525   +/-   ##
=======================================
  Coverage   11.17%   11.17%           
=======================================
  Files          83       83           
  Lines       32599    32599           
=======================================
  Hits         3642     3642           
  Misses      28458    28458           
  Partials      499      499           

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 fa0fdbe...fa0fdbe. Read the comment docs.

Copy link
Member

@simster7 simster7 left a comment

Choose a reason for hiding this comment

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

Some initial comments on this PR

@@ -0,0 +1,162 @@
# Cluster Workflow Templates
Copy link
Member

Choose a reason for hiding this comment

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

Not sure why this needs to be an entire new doc, especially since it's mostly copy/paste from the other doc. Could you please integrate the new information found here to docs/workflow-templates.md?

Copy link
Member Author

Choose a reason for hiding this comment

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

I initially thought of adding a section in workflow-template.md. But after I saw workflow-template.md which is very focused on explaining workflow-template and also it has a lot of information. It would be better to have it as a separate file and simply focus on one thing cluster-workflow-template. It has a link to workflow-template.md so Users can go to workflow-template information.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, sounds good 🙂

api/openapi-spec/swagger.json Outdated Show resolved Hide resolved
@@ -27,6 +30,7 @@ const routes: {
} = {
[workflowsUrl]: {component: workflows.component},
[workflowTemplatesUrl]: {component: workflowTemplates.component},
[clusterWorkflowTemplatesUrl]: {component: clusterWorkflowTemplates.component},
Copy link
Member

Choose a reason for hiding this comment

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

Should this be under the WorkflowTempaltes tab instead of its own tab? I'm not sure either way. Why do you think it should be its own?

Copy link
Member Author

Choose a reason for hiding this comment

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

I posted my feedback on slack for UI changes. I had a discussion with @alexmt to have a tab on-page to switch between workflow-template and cluster-workflow-template. It requires a lot of refactoring on UI module. He suggested having a separate page will be a clean way and also it is easy to implement. I do agree with that. I don't want to overload the workflow-template page with multiple options.

Copy link
Member

Choose a reason for hiding this comment

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

Good point. I agree with this.

Copy link
Member

Choose a reason for hiding this comment

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

Good point, agreed.

ui/src/app/shared/components/resource-submit.tsx Outdated Show resolved Hide resolved
@simster7 simster7 self-assigned this Apr 3, 2020
Copy link
Member

@simster7 simster7 left a comment

Choose a reason for hiding this comment

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

Just one more comment regarding the doc. Also make sure to run make lint; the CI is not catching that.


`WorkflowTemplate` documentation [link](./workflow-template.md)

## Defining `ClusterWorkflowTemplate`
Copy link
Member

Choose a reason for hiding this comment

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

One example here should be enough. Don't want to add too much redundant information

@@ -0,0 +1,162 @@
# Cluster Workflow Templates
Copy link
Member

Choose a reason for hiding this comment

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

Sure, sounds good 🙂

@@ -27,6 +30,7 @@ const routes: {
} = {
[workflowsUrl]: {component: workflows.component},
[workflowTemplatesUrl]: {component: workflowTemplates.component},
[clusterWorkflowTemplatesUrl]: {component: clusterWorkflowTemplates.component},
Copy link
Member

Choose a reason for hiding this comment

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

Good point, agreed.

@sarabala1979 sarabala1979 merged commit 59f746e into argoproj:master Apr 4, 2020
@simster7 simster7 mentioned this pull request Apr 5, 2020
@alexec alexec mentioned this pull request Apr 6, 2020
24 tasks
@sarabala1979
Copy link
Member Author

#2525

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants