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

feat: use name instead of index for instance id tag #135

Merged
merged 3 commits into from
Sep 1, 2022

Conversation

spoukke
Copy link
Contributor

@spoukke spoukke commented Aug 2, 2022

Currently, the tag used for instance id is the index of the job. But, if you run multiple K6 resources at the same time, there is no way to distinguish them.

Here I propose to use the name of the job, instead of the index of the job

@CLAassistant
Copy link

CLAassistant commented Aug 2, 2022

CLA assistant check
All committers have signed the CLA.

@yorugac
Copy link
Collaborator

yorugac commented Aug 14, 2022

Hi @spoukke,
Thanks for the PR and sorry for the delay! We cannot change instance_id: the whole point of instance_id tag is to distinguish between pods, not jobs. I suggest changing it to job_name tag instead and leaving instance_id as is. Would it work for your case?

@spoukke
Copy link
Contributor Author

spoukke commented Aug 18, 2022

Yes, it would work, I am going to do the modification!

Copy link
Collaborator

@yorugac yorugac left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Thank you @spoukke!

@yorugac
Copy link
Collaborator

yorugac commented Aug 19, 2022

But not the unit tests, apparently: https://github.com/grafana/k6-operator/runs/7917488949?check_suite_focus=true
They check the command and new tag is part of the command. Please check.

@spoukke
Copy link
Contributor Author

spoukke commented Aug 22, 2022

Hi @yorugac ! Just updated the unit tests

@spoukke spoukke requested a review from yorugac August 23, 2022 13:12
Copy link
Collaborator

@yorugac yorugac left a comment

Choose a reason for hiding this comment

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

@spoukke Thanks for following up with this! Merging in now.

@yorugac yorugac merged commit 02dfbc9 into grafana:main Sep 1, 2022
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