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

[Feature-10604] Task name in the copied and imported workflow without copy suffix #10607

Merged
merged 2 commits into from
Jul 26, 2022

Conversation

zhuxt2015
Copy link
Contributor

@zhuxt2015 zhuxt2015 commented Jun 24, 2022

Purpose of the pull request

close #10604

@SbloodyS SbloodyS changed the title Task name in the copied workflow without copy suffix [Feature-10604] Task name in the copied workflow without copy suffix Jun 24, 2022
@SbloodyS SbloodyS added feature new feature backend labels Jun 24, 2022
@SbloodyS SbloodyS added this to the 3.0.0-beta-3 milestone Jun 24, 2022
@SbloodyS
Copy link
Member

In dev, we split the task instance from the workflow instance. After the task instance is copied, the user will query two task definitions with the same name and content, which will cause some confusion. I think we should discuss about it.

@sonarcloud
Copy link

sonarcloud bot commented Jun 24, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@zhuxt2015
Copy link
Contributor Author

In dev, we split the task instance from the workflow instance. After the task instance is copied, the user will query two task definitions with the same name and content, which will cause some confusion. I think we should discuss about it.

In my opinion, I only care about whether the tasks under my workflow are successfully executed, and I don't care much about whether the name is duplicated with other workflows.

Instead, copy suffix redundant, but removing the copy suffix is a waste of time for me.

I've also encountered multiple copy suffixes in task name that caused copy failed/
image

In addition, if the user wants to avoid duplication, the task name can be modified manually.

@zhongjiajie
Copy link
Member

It is incompatible change and we need dissuss about it. Maybe some users already have some function base on this behavior.

If we accept this, please notice that we have import function will also add suffix to it

@zhongjiajie
Copy link
Member

BTW, it make no sence to me with your word

In my opinion, I only care about whether the tasks under my workflow are successfully executed, and I don't care much about whether the name is duplicated with other workflows.

So the name it is not important in your cases

Instead, copy suffix redundant, but removing the copy suffix is a waste of time for me.
In addition, if the user wants to avoid duplication, the task name can be modified manually.

Seem are opposing viewpoints

@zhongjiajie
Copy link
Member

I've also encountered multiple copy suffixes in task name that caused copy failed

But we may take more care about this one

@zhongjiajie
Copy link
Member

If we want to remove the copy and import suffix, we should add some incompatible notice in our release note

@zhuxt2015 zhuxt2015 force-pushed the copied_task_name_without_copy_suffix branch from 61de76c to 19c9e31 Compare July 26, 2022 02:25
@zhuxt2015 zhuxt2015 changed the title [Feature-10604] Task name in the copied workflow without copy suffix [Feature-10604] Task name in the copied and imported workflow without copy suffix Jul 26, 2022
Copy link
Member

@SbloodyS SbloodyS left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@ruanwenjun ruanwenjun left a comment

Choose a reason for hiding this comment

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

LGTM

@codecov-commenter
Copy link

codecov-commenter commented Jul 26, 2022

Codecov Report

Merging #10607 (19c9e31) into dev (4d9db34) will not change coverage.
The diff coverage is 0.00%.

@@            Coverage Diff            @@
##                dev   #10607   +/-   ##
=========================================
  Coverage     40.49%   40.49%           
+ Complexity     4887     4885    -2     
=========================================
  Files           951      951           
  Lines         37201    37201           
  Branches       4080     4081    +1     
=========================================
  Hits          15064    15064           
  Misses        20614    20614           
  Partials       1523     1523           
Impacted Files Coverage Δ
...api/service/impl/ProcessDefinitionServiceImpl.java 31.85% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us.

@sonarcloud
Copy link

sonarcloud bot commented Jul 26, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@SbloodyS SbloodyS merged commit 5ebd538 into apache:dev Jul 26, 2022
@zhongjiajie
Copy link
Member

Did we want to add this to version 3.0.0? @SbloodyS @caishunfeng

@SbloodyS
Copy link
Member

Did we want to add this to version 3.0.0? @SbloodyS @caishunfeng

I'm not quite sure about it.

zhongjiajie added a commit to zhongjiajie/dolphinscheduler that referenced this pull request Jul 26, 2022
Add incompatibles and give some hint to user when then
want to upgrade to specific version.

ref: apache#10607
zhongjiajie added a commit that referenced this pull request Jul 27, 2022
Add incompatibles and give some hint to user when then
want to upgrade to specific version.

ref: #10607
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.

[Feature][Api] Task name in the copied workflow without copy suffix
5 participants