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

ADRs: 007-011 - Adding Impact Sections #1456

Merged
merged 11 commits into from
Oct 24, 2024

Conversation

tjohnson7021
Copy link
Contributor

@tjohnson7021 tjohnson7021 commented Oct 18, 2024

Impact section completion for ADRs 007-011

This PR covers filling in the impact sections for ADRs 007-011 so that it is understood why the decision was made at the time and what the concerns were.

Issue #1247

Checklist

  • I have updated the documentation accordingly

- added Impact section
- updated content and formatting
- updated for clarity and formatting
- filled Impact section
- filled Impact section
- updated formatting
- filled in Impact section
- updated formatting
- filled in Impact section for 011
- updated formatting
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

1247 - Partially compliant

Fully compliant requirements:

  • Complete the "Impact" section for previously completed ADRs.

Not compliant requirements:

  • PRs should be created for groups of ADRs (like 5 max) for cleaner review.
⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Redundant Information
The added text in the Impact section contains redundant information which could be streamlined for clarity and conciseness.

Complexity Concern
The added details about Locust.io's scalability might be overly complex and could benefit from simplification to maintain focus on key impacts.

adr/007-software-bill-of-materials.md Outdated Show resolved Hide resolved
adr/008-load-testing.md Show resolved Hide resolved
adr/009-docker.md Show resolved Hide resolved
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Score
Enhancement
Enhance clarity and professionalism by removing a redundant phrase

Remove the redundant phrase "for a smoother read." from the decision section to
enhance clarity and professionalism.

adr/007-software-bill-of-materials.md [7]

-In the context of generating a secure modern application, facing the need to monitor application dependencies and generate a reliable SBOM, we decided to use the [CycloneDX Gradle Plugin](https://github.com/CycloneDX/cyclonedx-gradle-plugin#usage) and we chose not to use [Anchore/Syft](https://github.com/anchore/syft#supported-ecosystems) or Snyk FOSSID" for a smoother read.for SBOM generation at release of a version build, accepting that an SBOM needs to be generated during the CI/CD process
+In the context of generating a secure modern application, facing the need to monitor application dependencies and generate a reliable SBOM, we decided to use the [CycloneDX Gradle Plugin](https://github.com/CycloneDX/cyclonedx-gradle-plugin#usage) and we chose not to use [Anchore/Syft](https://github.com/anchore/syft#supported-ecosystems) or Snyk FOSSID for SBOM generation at release of a version build, accepting that an SBOM needs to be generated during the CI/CD process
Suggestion importance[1-10]: 10

Why: The suggestion correctly identifies and removes a redundant and unprofessional phrase from a formal decision document, significantly enhancing the clarity and professionalism of the text.

10
Correct grammatical errors to enhance readability

Correct the grammar by changing "supports integration with Anchore/Syft for SBOM
format conversion, providing flexibility to meet different ecosystem requirements"
to "supports the integration with Anchore/Syft for SBOM format conversion, providing
flexibility to meet different ecosystem requirements."

adr/007-software-bill-of-materials.md [30]

-**Format Conversion:** supports integration with Anchore/Syft for SBOM format conversion, providing flexibility to meet different ecosystem requirements
+**Format Conversion:** supports the integration with Anchore/Syft for SBOM format conversion, providing flexibility to meet different ecosystem requirements
Suggestion importance[1-10]: 8

Why: This suggestion improves grammatical structure by adding a necessary article, which enhances the readability and correctness of the document.

8
Clarify the context of manual script writing in the negative impact section

Clarify the negative impact by specifying that "Manual script writing in Locust.io
could be more time-consuming compared to GUI-based test creation offered by tools
like JMeter or Artillery.io."

adr/008-load-testing.md [37]

-**Manual Script Writing:** Test scripts need to be manually written, which could be more time-consuming compared to GUI-based test creation offered by tools like JMeter or Artillery.io.
+**Manual Script Writing:** Manual script writing in Locust.io could be more time-consuming compared to GUI-based test creation offered by tools like JMeter or Artillery.io.
Suggestion importance[1-10]: 6

Why: The suggestion adds specificity to the description of manual script writing, clarifying the context and making the negative impact more understandable.

6
Improve clarity by specifying the role of Kubernetes in Docker's scalability

Specify that "Docker supports scaling applications easily through container
orchestration tools like Kubernetes, which allows for better resource management and
load handling."

adr/009-docker.md [30]

-**Scalability:** Docker supports scaling applications easily through container orchestration tools like Kubernetes, allowing for better resource management and load handling.
+**Scalability:** Docker supports scaling applications easily through container orchestration tools like Kubernetes, which allows for better resource management and load handling.
Suggestion importance[1-10]: 5

Why: This suggestion slightly improves the clarity of the sentence by changing "allowing" to "which allows," which makes the causal relationship clearer, though the overall impact on the document is minor.

5

adr/007-software-bill-of-materials.md Outdated Show resolved Hide resolved
adr/009-docker.md Outdated Show resolved Hide resolved
adr/011-dast.md Outdated Show resolved Hide resolved
@tjohnson7021 tjohnson7021 mentioned this pull request Oct 22, 2024
30 tasks
- **Vulnerability Scanning:** With the ability to integrate with tools like [Anchore/Grype](https://github.com/anchore/grype#recommended), one can easily add vulnerability scanning to the process.


- **Format Conversion:** supports integration with Anchore/Syft for SBOM format conversion, providing flexibility to meet different ecosystem requirements
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick, but 'supports' is lowercase when the other points are capitalized.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Help me with this @pluckyswan - Some of the bullets read like a sentence if you remove the colon (like the one you've pointed out here. Should I just add a period so it looks more like a sentence or rephrase ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think rephrasing might help it come across more as a sentence

@@ -1,9 +1,12 @@
# 11. OWASP ZAP Dynamic Application Security Testing
# 11. OWASP ZAP Dynamic Application Security Testing (DAST)
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the criteria for what's abbreviated here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pluckyswan - I didn't touch too much of the top sections unless they just didn't explain enough. As long as the acronym was expanded on somewhere at the top of the doc, I left it alone. Also, In this case OWASP and ZAP are expanded under the decision section. Open to suggested changes though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was just curious since the IaC terraform ADR had both, but if it's expanded somewhere in the file it's probably good :)

adr/009-docker.md Outdated Show resolved Hide resolved
- applying changes suggested in the PR
Copy link
Member

@halprin halprin left a comment

Choose a reason for hiding this comment

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

I approve, but there is an extraneous quote.

adr/007-software-bill-of-materials.md Outdated Show resolved Hide resolved
remove quote

Co-authored-by: halprin <halprin@users.noreply.github.com>
Copy link

Copy link
Contributor

@pluckyswan pluckyswan left a comment

Choose a reason for hiding this comment

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

Good stuff

@tjohnson7021 tjohnson7021 merged commit cd778c1 into main Oct 24, 2024
17 checks passed
@tjohnson7021 tjohnson7021 deleted the devex/1247-add_impact_sections_to_adrs_007-011 branch October 24, 2024 17:13
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