Skip to content

Branch feature/amqp created. Customized the existing AMQP extension t… #1961

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

Closed
wants to merge 1 commit into from

Conversation

prmuthu
Copy link

@prmuthu prmuthu commented Jun 16, 2025

…o compatible with k6 0.57.0 and above versions.It supports AMQP1.0

What?

Customized the existing AMQP extension to compatible with k6 0.57.0 and above versions.It supports AMQP1.0

Checklist

  • [ yes] I have used a meaningful title for the PR.
  • [ yes] I have described the changes I've made in the "What?" section above.
  • [ yes] I have performed a self-review of my changes.
  • [ yes] I have run the npm start command locally and verified that the changes look good.
  • I have made my changes in the docs/sources/k6/next folder of the documentation.
  • I have reflected my changes in the docs/sources/k6/v{most_recent_release} folder of the documentation.
  • I have reflected my changes in the relevant folders of the two previous k6 versions of the documentation (if still applicable to previous versions).

Related PR(s)/Issue(s)

…o compatible with k6 0.57.0 and above versions.It supports AMQP1.0
@prmuthu prmuthu requested review from heitortsergent and a team as code owners June 16, 2025 08:47
@prmuthu prmuthu requested review from ankur22 and codebien and removed request for a team June 16, 2025 08:47
@CLAassistant
Copy link

CLAassistant commented Jun 16, 2025

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@codebien codebien left a comment

Choose a reason for hiding this comment

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

Hey @prmuthu, can you please sign the CLA? It is a mandatory step to get this pull request reviewed and merged.

@prmuthu prmuthu requested a review from codebien June 16, 2025 14:19
@heitortsergent heitortsergent requested a review from szkiba June 16, 2025 17:52
@heitortsergent
Copy link
Collaborator

@prmuthu thanks for opening this PR!

I've added @szkiba here to double-check, since this is related to extensions. I think the changes to the index.md files are ok, but would you mind reverting the changes to the explore.md files? Those are automatically updated by our extension registry, so when the extension is updated, it should automatically open a PR to update the docs so they're both in sync.

Copy link
Contributor

@szkiba szkiba left a comment

Choose a reason for hiding this comment

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

  • Leave explore.md as is, please.
  • Please keep the link to the official extension as is.

Copy link
Contributor

@szkiba szkiba left a comment

Choose a reason for hiding this comment

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

I've reviewed it again and would like to summarize my observations here, because the multiple comment threads are starting to get a bit chaotic.

  1. explore.md is automatically generated after changes to grafana/k6-extension-registry. As a result, this PR cannot be merged as long as explore.md is included in it.

  2. The forked repository has been registered in grafana/k6-extension-registry. (PR merged).

  3. I agree with @heitortsergent , in _index.md it would be better to reference a protocol extension that Grafana maintains and is actively maintained. I recommend the grafana/xk6-dns extension for this purpose. That is, instead of grafana/xk6-amqp, the grafana/xk6-dns extension should be referenced. Consequently, the _index.md modification should also be removed from this PR. (i.e. nothing will remain in the PR).

In summary:

@prmuthu
Copy link
Author

prmuthu commented Jun 26, 2025

@szkiba ,

Agree with you, please clarify my doubts before proceed.

we should open another PR (@heitortsergent ?) in which instead of the grafana/xk6-amqp extension, the grafana/xk6-dns extension is included as an example of a protocol extension in _index.md - How xk6-dns is related to amqp extension. Should I need to fork xk6-dns extension and incorporate the amqp changes? If so, how the users come to know the AMQP changes available with xk6-dns repo
this PR should be closed without merging - will close it.
the automatically generated PR #1965 should be merged - I am unable to merge it, could you advise here

@szkiba
Copy link
Contributor

szkiba commented Jun 26, 2025

@szkiba ,

Agree with you, please clarify my doubts before proceed.

we should open another PR (@heitortsergent ?) in which instead of the grafana/xk6-amqp extension, the grafana/xk6-dns extension is included as an example of a protocol extension in _index.md - How xk6-dns is related to amqp extension. Should I need to fork xk6-dns extension and incorporate the amqp changes? If so, how the users come to know the AMQP changes available with xk6-dns repo this PR should be closed without merging - will close it. the automatically generated PR #1965 should be merged - I am unable to merge it, could you advise here

Hi @prmuthu , you don't need to do anything, the PR should be created and merged by the k6-docs maintainers (the other one).

@codebien
Copy link
Contributor

@szkiba if we have this case then we need to implement an automated check that block pull requests that attempt to modify the explore file. Something like https://github.com/marketplace/actions/prevent-file-change

@heitortsergent
Copy link
Collaborator

@prmuthu just want to add to @szkiba review comments, thanks for opening this PR!

Like he said, I think it's better if I open a separate PR to update the Extensions page and update the text there, and the AMQP extension should be added to the list by the separate PR mentioned here.

I'm going to go ahead and close this PR, thanks again for creating an extension and contributing to the docs. 🙇

@heitortsergent
Copy link
Collaborator

@codebien I'll look into adding the GH action you linked to, so hopefully this prevents confusion in the future. 🙇

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.

5 participants