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

[Implementation] (Proposal 2) Add Benchmark Pipeline #127

Merged
merged 5 commits into from
Nov 28, 2024

Conversation

dipankardas011
Copy link
Contributor

@dipankardas011 dipankardas011 commented Nov 14, 2024

What type of PR is this?

kind/feature

What this PR does / why we need it:

It will give us a alternative route to use kubernetes manifests for the benchmark jobs instead of reusing github workflow.

With this we are trying to use the manifest location in github or any public facing endpoint from where we can get them

Which issue(s) this PR fixes:

Addresses part of #121

Special notes for your reviewer (optional):

Created this for finding alternative route for the current proposal 2 impl

Refer: #124, #121 (comment)

@dipankardas011 dipankardas011 force-pushed the iterate/121 branch 2 times, most recently from b0a97ab to 6b08f69 Compare November 15, 2024 18:28
@dipankardas011 dipankardas011 marked this pull request as ready for review November 15, 2024 18:29
@dipankardas011 dipankardas011 requested a review from a team as a code owner November 15, 2024 18:29
Copy link
Contributor

@rossf7 rossf7 left a comment

Choose a reason for hiding this comment

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

@dipankardas011 Looks good just a few minor comments. We should also update proposal 2 since this changes the design.

projects/projects.json Outdated Show resolved Hide resolved
.github/workflows/benchmark-pipeline.yaml Outdated Show resolved Hide resolved
.github/workflows/benchmark-pipeline.yaml Outdated Show resolved Hide resolved
.github/workflows/benchmark-pipeline.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@rossf7 rossf7 left a comment

Choose a reason for hiding this comment

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

@dipankardas011 Some more renaming is needed due to the param name change.

Please check if any more renaming is required and then I think this is good to go.

scripts/project-trigger.sh Outdated Show resolved Hide resolved
scripts/project-trigger.sh Outdated Show resolved Hide resolved
Copy link
Contributor

@rossf7 rossf7 left a comment

Choose a reason for hiding this comment

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

LGTM

I created a fix for CI issue in #130

It will give us a alternative route to use kubernetes manifests for the
benchmark jobs instead of reusing github workflow.

With this we are trying to use the manifest location in github or any
public facing endpoint from where we can get them

Signed-off-by: Dipankar Das <[email protected]>
Signed-off-by: Dipankar Das <[email protected]>
it will help us to exactly point which commit this file belongs to and
avoids updates to the file in the specific branch and we accidentally use
wrong or maliciously crafted manifest

Signed-off-by: Dipankar Das <[email protected]>
@nikimanoledaki
Copy link
Contributor

nikimanoledaki commented Nov 28, 2024

@dipankardas011 could you rebase with main to get CI to pass please? :)

EDIT: sorry, just saw that you did that already and it's green! 👍 I hadn't refreshed my page 🙈

@nikimanoledaki nikimanoledaki merged commit 9c058d0 into cncf-tags:main Nov 28, 2024
1 check passed
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.

3 participants