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

Module 3 review #18

Merged
merged 24 commits into from
Dec 30, 2022
Merged

Module 3 review #18

merged 24 commits into from
Dec 30, 2022

Conversation

immavalls
Copy link
Collaborator

  • Mostly fixed minor format issues, typos, and broken links.
  • I've refactored the location for module 08, as it was difficult to follow when navigating directly.

Note: I'm leaving for another PR the changes to https://github.com/grafana/k6-learn/blob/main/Modules/III-k6-Intermediate/08-Setting-load-profiles-with-executors/Ramping-Arrival-Rate-Exercises.md#autoscaling-of-virtual-users and https://github.com/grafana/k6-learn/blob/main/Modules/III-k6-Intermediate/08-Setting-load-profiles-with-executors/Constant-Arrival-Rate-Exercises.md#autoscaling-of-virtual-users. We should probably discourage the use of maxVUs in favor of allocating the required number of VUs, and explain the executor behavior a bit better here, and in the docs.

@immavalls immavalls changed the title Imma module 3 review Module 3 review Dec 29, 2022
Copy link
Contributor

@MattDodsonEnglish MattDodsonEnglish left a comment

Choose a reason for hiding this comment

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

Thanks so much for the dust up. I appreciate the commitment to that for unrestricted clauses. I thought I was the only one who cared about this important issue 😁

I just left some nits, the vast majority of which weren't even from the suggested changes, but just happened to be in the relevant lines.

Since these are all grammar and formatting, I'm going to say that we can merge this without Nicole's review. For any load testing related changes, though, it's definitely better to get a review from the experts.


To correct this error and accurately mimic user behavior, the script must:
- Save the response to the request in Step 1.
- Extract the csrf token from Step 1.
- Pass the csrf token when requesting Step 2.
- Extract the [Cross-Site Request Forgery (CSFR)](https://owasp.org/www-community/attacks/csrf) token from Step 1.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Extract the [Cross-Site Request Forgery (CSFR)](https://owasp.org/www-community/attacks/csrf) token from Step 1.
- Extract the [Cross-Site Request Forgery (CSFR)](https://owasp.org/www-community/attacks/csrf) token from Step 1.

Is this the first place that CSFR appears? I think we might want to link it there, or not link unless it's necessary here. Generally, I try to avoid links in direct procedures, since links naturally call to themselves, which may divide the reader's attention.

Willing to change my opinion though if the info is particularly useful here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it was the first appearance, and for newcomers it's good to explain what this is (unless we had a glossary)

Matt's review

Co-authored-by: Matt Dodson <47385188+MattDodsonEnglish@users.noreply.github.com>
@immavalls immavalls merged commit e4853c5 into main Dec 30, 2022
@immavalls immavalls deleted the imma-review-module-3 branch December 30, 2022 08:14
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.

2 participants