Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

Added validation check to impose limits on stored, free-form strings #180

Merged
merged 13 commits into from
Aug 18, 2021
Merged

Added validation check to impose limits on stored, free-form strings #180

merged 13 commits into from
Aug 18, 2021

Conversation

yindia
Copy link
Contributor

@yindia yindia commented Apr 13, 2021 •
edited
Loading

  • Added client side validation in flyteadmin
  • Added 255 char limit in all string field

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

How did you fix the bug, make the feature etc. Link to any design docs etc

Tracking Issue

flyteorg/flyte#336
flyteorg/flyte#896

Follow-up issue

NA
OR
https://github.com/lyft/flyte/issues/

@yindia
added validation for project name
505becd 
Signed-off-by: yuvraj <evalsocket@gmail.com>
@codecov
Copy link

codecov bot commented Apr 13, 2021 •
edited
Loading

Codecov Report

Merging #180 (428cbc6) into master (e6e6576) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #180      +/-   ##
==========================================
- Coverage   57.15%   57.14%   -0.01%     
==========================================
  Files         135      135              
  Lines       10159    10160       +1     
==========================================
  Hits         5806     5806              
- Misses       3704     3705       +1     
  Partials      649      649
Flag Coverage Δ
unittests 55.94% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/repositories/config/postgres.go 48.00% <0.00%> (-2.00%) ⬇️

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 e6e6576...428cbc6. Read the comment docs.

yindia added 2 commits April 13, 2021 23:39
@yindia
Added validation check for all models
a2c6be6 
Signed-off-by: yuvraj <evalsocket@gmail.com>
@yindia
fix testcase
dc962bd 
Signed-off-by: yuvraj <evalsocket@gmail.com>
@yindia yindia changed the title [WIP] Added validation check to impose limits on stored, free-form strings Added validation check to impose limits on stored, free-form strings Apr 13, 2021
@yindia
Copy link
Contributor Author

yindia commented Apr 16, 2021

@kumare3 I don't need to list all string fields...you can comment on my PR and I will update the changes
cc @katrogan @EngHabu

@kumare3
Copy link
Contributor

kumare3 commented Apr 17, 2021

Aah ya sure

@yindia
Copy link
Contributor Author

yindia commented Apr 24, 2021

@kumare3 Any update on this ?

@kumare3
Copy link
Contributor

kumare3 commented Apr 24, 2021

Will look tomorrow

@kumare3
Copy link
Contributor

kumare3 commented Apr 28, 2021

@evalsocket this can be a very dangerous change. I would need reviews from @anandswaminathan / @katrogan / @kanterov

kanterov
kanterov reviewed May 3, 2021
View reviewed changes
tests/launch_plan_test.go Outdated Show resolved Hide resolved
@kanterov
Copy link

kanterov commented May 3, 2021

Does length(3|50) means that there is a lower bound of 3? What is the reason for that?

@yindia
Copy link
Contributor Author

yindia commented May 23, 2021

@kanterov 3 is lower bound and we can remove it If we don't want it.

yindia added 2 commits May 23, 2021 13:34
@yindia
Updated string limit
70409be 
Signed-off-by: yuvraj <evalsocket@gmail.com>
@yindia
Rebase
4323cd4 
Signed-off-by: Yuvraj <evalsocket@gmail.com>
@yindia yindia marked this pull request as ready for review May 23, 2021 08:09
@yindia yindia requested review from kanterov and kumare3 May 23, 2021 08:09
@yindia
Copy link
Contributor Author

yindia commented May 23, 2021

@katrogan @kumare3 @anandswaminathan @kanterov need your review?

@yindia
update go.sum
3ecdb0c 
Signed-off-by: Yuvraj <evalsocket@gmail.com>
@yindia
set lower limit value for validation
35e6fa2 
Signed-off-by: Yuvraj <evalsocket@gmail.com>
@yindia yindia marked this pull request as draft May 27, 2021 10:44
@yindia
Debug log
e0113ee 
Signed-off-by: Yuvraj <evalsocket@gmail.com>
yindia and others added 2 commits May 30, 2021 16:02
@yindia
Debug log
a126614 
Signed-off-by: Yuvraj <evalsocket@gmail.com>
@yindia
Merge branch 'master' into fix-336
74e728b
@yindia
Copy link
Contributor Author

yindia commented May 30, 2021

@anandswaminathan As per your comment. I added an upper limit to 255 char as a uniform value rather than 50 char.
CC: @kumare3 @kanterov

@yindia yindia marked this pull request as ready for review May 31, 2021 12:51
EngHabu
EngHabu reviewed Aug 3, 2021
View reviewed changes
go.sum Show resolved Hide resolved
pkg/repositories/models/node_execution.go Outdated Show resolved Hide resolved
@yindia yindia requested a review from EngHabu August 13, 2021 03:25
EngHabu
EngHabu previously approved these changes Aug 13, 2021
View reviewed changes
go.sum Outdated Show resolved Hide resolved
@yindia yindia requested a review from EngHabu August 17, 2021 15:42
@yindia yindia merged commit e79cd34 into flyteorg:master Aug 18, 2021
eapolinario pushed a commit that referenced this pull request Sep 6, 2023
@yindia
Added validation check to impose limits on stored, free-form strings (#…
c06bbfd 
…180)

* Added validation check for all models

Signed-off-by: Yuvraj <evalsocket@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@EngHabu EngHabu EngHabu approved these changes

@wild-endeavor wild-endeavor Awaiting requested review from wild-endeavor wild-endeavor is a code owner

@kanterov kanterov Awaiting requested review from kanterov

@kumare3 kumare3 Awaiting requested review from kumare3 kumare3 is a code owner

@katrogan katrogan Awaiting requested review from katrogan katrogan is a code owner

@anandswaminathan anandswaminathan Awaiting requested review from anandswaminathan

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@yindia @kumare3 @kanterov @EngHabu @katrogan @anandswaminathan