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

Fixing some of the linting errors #176

Merged
merged 18 commits into from
Nov 15, 2024
Merged

Conversation

CsatariGergely
Copy link
Contributor

This is still in the works, do not approve it. I just need the Vale job to run.

Signed-off-by: Gergely Csatari <[email protected]>
Signed-off-by: Gergely Csatari <[email protected]>
@nephio-prow nephio-prow bot requested review from liamfallon and s3wong October 5, 2024 17:25
Copy link

netlify bot commented Oct 5, 2024

Deploy Preview for nephio ready!

Name Link
🔨 Latest commit 8c8b7df
🔍 Latest deploy log https://app.netlify.com/sites/nephio/deploys/67365e9db8eb990008c2ab57
😎 Deploy Preview https://deploy-preview-176--nephio.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

@s3wong s3wong left a comment

Choose a reason for hiding this comment

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

/lgtm

@nephio-prow nephio-prow bot added the lgtm label Oct 6, 2024
@s3wong
Copy link

s3wong commented Oct 6, 2024

@CsatariGergely vale seems to complain on some capitalization (and couple on choice of wording) errors. PTAL.

@CsatariGergely
Copy link
Contributor Author

@CsatariGergely vale seems to complain on some capitalization (and couple on choice of wording) errors. PTAL.

Yep, it is a massive list :)

I will also configure it not to fail the check, but only post warnings. I would like to use these as more like recommendations for now.

Signed-off-by: Gergely Csatari <[email protected]>
@nephio-prow nephio-prow bot removed the lgtm label Oct 6, 2024
@CsatariGergely
Copy link
Contributor Author

Now it is strange, that the Vale check is not triggered for every update.

Signed-off-by: Gergely Csatari <[email protected]>
@@ -129,11 +129,11 @@ The following limitations need to be borne in mind:
does not match src line`.
If such a message appears, then retry in a little while, as this error may resolve itself. Restarting
Porch may also help.
* During specialization, we may have duplicate parameterRef extensions, leading to failed deployments
* During specialization, we may have duplicate parameterRef extensions, leading to unsuccesfull deployments
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* During specialization, we may have duplicate parameterRef extensions, leading to unsuccesfull deployments
* During specialization, we may have duplicate parameterRef extensions, leading to unsuccesful deployments

Copy link
Member

Choose a reason for hiding this comment

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

I don't know why the word "unsuccessful" is spelt that way in English (it makes no sense) :-D

Copy link

Choose a reason for hiding this comment

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

Actually I don't know why our doc checking/lint tool would find "failed" deployments offensively, it is a much better wording to capture the reality than an "unsuccessful" deployment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I can change it back to failed, if you think that it is better in this context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is changed back now.

Signed-off-by: Gergely Csatari <[email protected]>
Signed-off-by: Gergely Csatari <[email protected]>
Signed-off-by: Gergely Csatari <[email protected]>
Signed-off-by: Gergely Csatari <[email protected]>
Signed-off-by: Gergely Csatari <[email protected]>
Signed-off-by: Gergely Csatari <[email protected]>
@CsatariGergely CsatariGergely changed the title Fixing some of the linting erorrs Fixing some of the linting errors Oct 23, 2024
@CsatariGergely
Copy link
Contributor Author

Now it is strange, that the Vale check is not triggered for every update.

This is fixed now.

- Moving the env statement in the GitHub workflow
- Printing the environment in the ocntext of vale
- Adding workds to the accept list
- Fixing findings

Signed-off-by: Gergely Csatari <[email protected]>
Signed-off-by: Gergely Csatari <[email protected]>
Signed-off-by: Gergely Csatari <[email protected]>
Signed-off-by: Gergely Csatari <[email protected]>
@CsatariGergely CsatariGergely changed the title Fixing some of the linting errors WIP: Fixing some of the linting errors Oct 25, 2024
@CsatariGergely CsatariGergely changed the title WIP: Fixing some of the linting errors Fixing some of the linting errors Nov 14, 2024
@CsatariGergely
Copy link
Contributor Author

/assign @arora-sagar @liamfallon

This is ready for merge now.

@efiacor
Copy link
Collaborator

efiacor commented Nov 15, 2024

/approve
/lgtm

Copy link
Contributor

nephio-prow bot commented Nov 15, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: efiacor

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@nephio-prow nephio-prow bot added the approved label Nov 15, 2024
@radoslawc radoslawc merged commit 2264136 into nephio-project:main Nov 15, 2024
9 checks passed
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.

6 participants