Repository navigation
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
… to trigger strict check
✅ Pull request preview available for checkingBuilt without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
@Caesarsage wdyt, adding a strict hugo check when making changes to netlify.toml file ? imho it might be quite beneficial, it is how I saw the ES community banner missing. any other files that can be monitored? thanks! :) |
|
Thanks @mblag, nice catch on the ES banner! A few things:
|
|
Also I suggest you keep only the $(HUGO_FLAGS) Makefile change and add it to production-build too. Then the Prow job in kubernetes/test-infra#36793 can run the strict check. Prow compares against main, so it doesn't skip pushes, and the preview still works if the strict check fails. |
|
thanks, @Caesarsage! I can keep this PR as a test one and open a new one (and tag you there) only with the changes that will go to main. Let me get familiar with the test-infra jobs and I will proceed further with the changes. I might ping you on slack in case I do not see the e2e pipeline as I am not a member of the k8s org, working towards it:) |
lmktfy
left a comment
There was a problem hiding this comment.
It's more complicated, but the correct place to put that new check is likely to be via Prow and a test job. See kubernetes/test-infra#36793 for an example of what the change might look like.
There was a problem hiding this comment.
I don't think that Netlify is the right place to run this kind of check.
|
If we have a non-required CI test that reveals whether a PR adds new warnings: great. But, again, more work. Let's file a new issue. Why? Because issues (not PRs) are where we discuss "what needs fixing?"; PRs are the place to discuss "is this fix the right change to close the issue". |
|
/remove-area localization |
|
+1 to filing an issue first. Agreed that Prow is the right place for this. The check can build on kubernetes/test-infra#36793. |
|
thanks, @lmktfy @Caesarsage. Tim, you are totally right. Here is the filed issue #59106 (my first one, yey) |
During CI deploy preview step and only during Hugo configuration or version updates run the best practices command
--panicOnWarningFirst CI run failed as expected as changes in the netlify.toml (add the panicOnWarning flag) where present and an ES localization issue (#59080) produces WARN for the hugo:
Second CI run succeeded after cherry picking the fix for the ES localization issue (#59080):
Third CI with standard deploy triggered with empty commit: