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

Fixed bug for incorrect name #4175

Merged
merged 12 commits into from
Oct 12, 2023

Conversation

Virtual4087
Copy link
Contributor

@Virtual4087 Virtual4087 commented Oct 5, 2023

Closes #4174

Tracking issue

Describe your changes

Check all the applicable boxes

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

Screenshots

Note to reviewers

@welcome
Copy link

welcome bot commented Oct 5, 2023

Thank you for opening this pull request! 🙌

These tips will help get your PR across the finish line:

  • Most of the repos have a PR template; if not, fill it out to the best of your knowledge.
  • Sign off your commits (Reference: DCO Guide).

@samhita-alla
Copy link
Contributor

@kumare3, do you think this PR can be merged?

@codecov
Copy link

codecov bot commented Oct 5, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (0ca2d22) 58.95% compared to head (31a0d69) 59.30%.

❗ Current head 31a0d69 differs from pull request most recent head 705044c. Consider uploading reports for the commit 705044c to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4175      +/-   ##
==========================================
+ Coverage   58.95%   59.30%   +0.34%     
==========================================
  Files         621      552      -69     
  Lines       52932    39798   -13134     
==========================================
- Hits        31206    23601    -7605     
+ Misses      19229    13864    -5365     
+ Partials     2497     2333     -164     
Flag Coverage Δ
unittests ?

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

Files Coverage Δ
...pkg/manager/impl/validation/execution_validator.go 73.80% <100.00%> (+1.73%) ⬆️

... and 557 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -131,7 +131,7 @@ func CheckValidExecutionID(executionID, fieldName string) error {
matched := executionIDRegex.MatchString(executionID)

if !matched {
return errors.NewFlyteAdminErrorf(codes.InvalidArgument, "invalid %s format: %s", fieldName, executionID)
return errors.NewFlyteAdminErrorf(codes.InvalidArgument, "invalid %s format: %s, does not match regex '^[a-z][a-z\-0-9]*$'", fieldName, executionID)
Copy link
Contributor

Choose a reason for hiding this comment

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

can we not copy paste the regex, instead print the regex value itself.
Also, update the unit tests

@Virtual4087
Copy link
Contributor Author

umm is it okay now?

@kumare3
Copy link
Contributor

kumare3 commented Oct 6, 2023

Lgtm

@Virtual4087
Copy link
Contributor Author

while you're at it can you add the hacktoberfest label as well?

@Virtual4087
Copy link
Contributor Author

can you share the test location? I'll update it

@Virtual4087
Copy link
Contributor Author

I'm not familiar with these tests so i couldn't find it but the changes work well. The test just needs to be updated

@Virtual4087
Copy link
Contributor Author

i think it's good now

Copy link
Member

@pingsutw pingsutw left a comment

Choose a reason for hiding this comment

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

Test is failing.

        	Error Trace:	/home/runner/work/flyte/flyte/flyteadmin/pkg/manager/impl/validation/execution_validator_test.go:45
        	Error:      	Error message not equal:
        	            	expected: "invalid name format: 12345"
        	            	actual  : "invalid name format: 12345, does not match regex '^[a-z][a-z\\-0-9]*$'"

Updated Test Validation for invalid name by adding the regex in the error message.

Signed-off-by: Shovit <[email protected]>
@Virtual4087 Virtual4087 requested a review from pingsutw October 8, 2023 03:33
@Virtual4087
Copy link
Contributor Author

Ok it should work now i updated the test

@samhita-alla
Copy link
Contributor

@Virtual4087, I'll add the hacktoberfest-accepted label once the PR is merged.

@Virtual4087
Copy link
Contributor Author

I think you guys need to update the unit test for the checks to be successful. The test expects "invalid name format: 12345" but you asked to make it ""invalid name format: 12345, does not match regex '^[a-z][a-z\-0-9]*$'". I've also updated the tests on my branch so kindly check the pr once.

Copy link
Member

@pingsutw pingsutw left a comment

Choose a reason for hiding this comment

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

Line 173 and 176 need to be updated as well.

assert.EqualError(t, err, "invalid a format: a_sdd")
err = CheckValidExecutionID("asd@", "a")
assert.NotNil(t, err)
assert.EqualError(t, err, "invalid a format: asd@")

@Virtual4087
Copy link
Contributor Author

Line 173 and 176 need to be updated as well.

assert.EqualError(t, err, "invalid a format: a_sdd")
err = CheckValidExecutionID("asd@", "a")
assert.NotNil(t, err)
assert.EqualError(t, err, "invalid a format: asd@")

Done

@samhita-alla
Copy link
Contributor

@pingsutw, could you please review this PR now?

Copy link
Member

@pingsutw pingsutw left a comment

Choose a reason for hiding this comment

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

LGTM

@pingsutw pingsutw merged commit 8854351 into flyteorg:master Oct 12, 2023
36 checks passed
@welcome
Copy link

welcome bot commented Oct 12, 2023

Congrats on merging your first pull request! 🎉

@Virtual4087
Copy link
Contributor Author

Finally

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.

[BUG] For incorrect name formats return a better error
4 participants