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.

Sign up for GitHub

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

Jump to bottom

Remove escape character to fix broken grafana dashboard configuration #2718

Merged
merged 1 commit into from
Mar 8, 2023
Merged

Remove escape character to fix broken grafana dashboard configuration #2718

merged 1 commit into from
Mar 8, 2023

Conversation

shaobo76
Copy link
Contributor

@shaobo76 shaobo76 commented Mar 2, 2023

This PR tries to fix the error in #2717. Verified in my local environment.

@volcano-sh-bot
Copy link
Contributor

Welcome @shaobo76!

It looks like this is your first PR to volcano-sh/volcano 馃帀.

Thank you, and welcome to Volcano. 😃

@volcano-sh-bot volcano-sh-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Mar 2, 2023
@wangyang0616
Copy link
Member

wangyang0616 commented Mar 3, 2023 •
edited
Loading

Please make ci happy. You can view the details failure reasons and repair methods through the detail of the failed ci

@shaobo76
Copy link
Contributor Author

shaobo76 commented Mar 3, 2023

Please make ci happy. You can view the details failure reasons and repair methods through the detail of the failed ci

Done. Thanks for pointing that out.

@wangyang0616
Copy link
Member

Can you help to trigger the running of ci test cases, thanks! @william-wang @Thor-wl

@Thor-wl
Copy link
Contributor

Thor-wl commented Mar 6, 2023

Can you help to trigger the running of ci test cases, thanks! @william-wang @Thor-wl

OK, triggered

hwdef
hwdef reviewed Mar 6, 2023
View reviewed changes
Copy link
Member

@hwdef hwdef left a comment

Choose a reason for hiding this comment

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

I think this file in generate by ./hack/generate-yaml.sh, you should modify it.

@shaobo76
Copy link
Contributor Author

shaobo76 commented Mar 6, 2023

I think this file in generate by ./hack/generate-yaml.sh, you should modify it.

Finished. And it seems that remove the escape character is the right way.

@shaobo76 shaobo76 changed the title Add escape to fix broken grafana dashboard configuration Remove escape character to fix broken grafana dashboard configuration Mar 6, 2023
@volcano-sh-bot volcano-sh-bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed lgtm Indicates that a PR is ready to be merged. labels Mar 7, 2023
@volcano-sh-bot volcano-sh-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 7, 2023
@volcano-sh-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Thor-wl

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

@shaobo76
Copy link
Contributor Author

shaobo76 commented Mar 8, 2023

Thank you for the approval. What can i do to pass those tests? @Thor-wl

hwdef
hwdef reviewed Mar 8, 2023
View reviewed changes
Copy link
Member

@hwdef hwdef left a comment

Choose a reason for hiding this comment

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

please squash your commit to one

@volcano-sh-bot volcano-sh-bot removed the lgtm Indicates that a PR is ready to be merged. label Mar 8, 2023
@shaobo76
Remove escape character to fix broken grafana dashboard configuration
d246424 
Signed-off-by: Shaobo Zhang <1171337+shaobo76@users.noreply.github.com>
@shaobo76
Copy link
Contributor Author

shaobo76 commented Mar 8, 2023 •
edited
Loading

please squash your commit to one

Finished.

@shaobo76 shaobo76 closed this Mar 8, 2023
@shaobo76 shaobo76 reopened this Mar 8, 2023
hwdef
hwdef reviewed Mar 8, 2023
View reviewed changes
Copy link
Member

@hwdef hwdef left a comment

Choose a reason for hiding this comment

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

/ok-to-test
/lgtm

@volcano-sh-bot volcano-sh-bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. lgtm Indicates that a PR is ready to be merged. labels Mar 8, 2023
@volcano-sh-bot volcano-sh-bot merged commit dffd0c3 into volcano-sh:master Mar 8, 2023
@wangyang0616
Copy link
Member

wangyang0616 commented Mar 9, 2023 •
edited
Loading

@shaobo76 After the current PR is merged, there is an error in the CI operation.

May I ask what content we have manually modified about grafana.yaml, can you describe it in detail? It is not easy for diffwenjian to see. Thanks!

The CI error message is as follows:
image

@wangyang0616 wangyang0616 mentioned this pull request Mar 9, 2023
Closed
@shaobo76
Copy link
Contributor Author

shaobo76 commented Mar 9, 2023 •
edited
Loading

Sorry for the trouble. I removed the '\' around '{{' to make it looks good as a grafana dashboard. But I didn't realize that it'll break the helm template command. The correct way to escape double curly brackets in Go templates is like {{ "{{queue}}" }} or {{ ` {{queue}} ` }}.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hwdef hwdef hwdef left review comments

@Thor-wl Thor-wl Thor-wl approved these changes

@merryzhou merryzhou Awaiting requested review from merryzhou

Assignees

@hwdef hwdef

@Thor-wl Thor-wl

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@shaobo76 @volcano-sh-bot @wangyang0616 @Thor-wl @hwdef