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

Added validation for project Name #166

Merged
merged 1 commit into from Mar 23, 2023

Conversation

hiteshwani29
Copy link
Contributor

@hiteshwani29 hiteshwani29 commented Feb 27, 2023

What does this PR change?

Does the PR depend on any other PRs or Issues? If yes, please list them.

Checklist

I confirm, that I have...

  • Read and followed the contributing guide in CONTRIBUTING.md
  • Added tests for this PR
  • Formatted the code using go fmt (if applicable)
  • Updated documentation on the Paralus docs site (if applicable)
  • Updated CHANGELOG.md

@hiteshwani29
Copy link
Contributor Author

please review this PR
@niravparikh05 @akshay196 @mabhi

@nirav-rafay nirav-rafay linked an issue Feb 28, 2023 that may be closed by this pull request
@niravparikh05 niravparikh05 added this to the v0.2.2 milestone Mar 3, 2023
Copy link
Contributor

@niravparikh05 niravparikh05 left a comment

Choose a reason for hiding this comment

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

@hiteshwani29 thanks for your pr, really appreciate your contribution towards paralus. There are a few minor comments, otherwise this looks good.

Also can you ensure you run go fmt before commiting your changes as gh actions are currently failing due to it.

Ensure to update changelog.md as well, you can follow the same format as in the file.

Let me know if you have any doubts.

pkg/common/constants.go Outdated Show resolved Hide resolved
pkg/service/project.go Outdated Show resolved Hide resolved
@niravparikh05
Copy link
Contributor

@hiteshwani29 Issue #149 is similar to this, it can use the same regex validations during cluster creation ? Will it be possible for you to make changes for the same with this pr ?

Copy link
Member

@akshay196 akshay196 left a comment

Choose a reason for hiding this comment

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

Some additional comments.

pkg/service/utils.go Outdated Show resolved Hide resolved
pkg/common/constants.go Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Mar 13, 2023

Codecov Report

Merging #166 (7f9a6be) into main (74f1928) will decrease coverage by 0.03%.
The diff coverage is 25.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main     #166      +/-   ##
==========================================
- Coverage   34.68%   34.66%   -0.03%     
==========================================
  Files          74       74              
  Lines       11671    11682      +11     
==========================================
+ Hits         4048     4049       +1     
- Misses       7075     7084       +9     
- Partials      548      549       +1     
Impacted Files Coverage Δ
pkg/service/project.go 55.89% <25.00%> (-0.23%) ⬇️

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@hiteshwani29 hiteshwani29 requested review from akshay196 and niravparikh05 and removed request for akshay196 and niravparikh05 March 13, 2023 13:59
@hiteshwani29
Copy link
Contributor Author

hiteshwani29 commented Mar 16, 2023

@hiteshwani29 Issue #149 is similar to this, it can use the same regex validations during cluster creation ? Will it be possible for you to make changes for the same with this pr ?

sorry @niravparikh05 I missed this comment but we already have the validation for the cluster name
https://github.com/paralus/paralus/blob/main/pkg/service/cluster.go#L124

@hiteshwani29
Copy link
Contributor Author

@niravparikh05 @akshay196
Please review the changes and let me know.

pkg/common/constants.go Outdated Show resolved Hide resolved
@akshay196
Copy link
Member

@hiteshwani29 One single change ⬆️ . Otherwise your code looks good to me.

@hiteshwani29 hiteshwani29 force-pushed the fix-projectName branch 2 times, most recently from e003141 to 6afd092 Compare March 17, 2023 11:54
akshay196
akshay196 previously approved these changes Mar 18, 2023
@hiteshwani29
Copy link
Contributor Author

hiteshwani29 commented Mar 18, 2023

@akshay196 I don't have merge access. Can you merge this PR and close the issue?

@akshay196
Copy link
Member

Waiting for @niravparikh05 to appove.

@hiteshwani29
Copy link
Contributor Author

Waiting for @niravparikh05 to appove.

ok @akshay196. will wait for him.

@niravparikh05
Copy link
Contributor

looks good to me, @hiteshwani29 need changelog update and we are good to merge.

@hiteshwani29
Copy link
Contributor Author

looks good to me, @hiteshwani29 need changelog update and we are good to merge.

Done @niravparikh05

@akshay196
Copy link
Member

@niravparikh05 After your approval, squash commits and change message to something like fix: add project name validation before merging. let's try conventional commits to achieve #177 from here.

Signed-off-by: hiteshwani29 <hiteshwani29@gmail.com>
Copy link
Contributor

@niravparikh05 niravparikh05 left a comment

Choose a reason for hiding this comment

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

lgtm

@akshay196 akshay196 merged commit 1e7b2ea into paralus:main Mar 23, 2023
4 checks passed
@akshay196
Copy link
Member

@hiteshwani29 Thanks for contributing and congratulation on your first PR. 🥳

@hiteshwani29
Copy link
Contributor Author

Thanks 🙏 @akshay196
Looking forward to make more contribution in paralus.

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.

Project Names With Slashes Allowed
4 participants