-
Notifications
You must be signed in to change notification settings - Fork 0
feat: Github workflow for a11y issues benchmarking #4
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
base: main
Are you sure you want to change the base?
Conversation
.github/workflows/A11y SLA.yml
Outdated
env: | ||
GITHUB_TOKEN: ${{ secrets.PERSONAL_ACCESS_TOKEN }} | ||
run: | | ||
set -e |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may want to define this code in a separate bash script (e.g. label_a11y_issues.sh
) instead of inline. It would probably be easier to maintain that way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one i am not sure how to do this? could you please help me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I'm suggesting is you:
- Create a file called
label_a11y_issues.sh
- Make it executable (e.g.
chmod +x label_a11y_issues.sh
- Move everything that's in the
run
block into the file and add a#!/bin/bash
to the top of the file. Example:
#!/bin/bash
set -e
....
- Update this to just run the script:
run: ./label_a11y_issues.sh
.github/workflows/A11y SLA.yml
Outdated
# Define SLA Thresholds | ||
declare -A LABEL_THRESHOLDS=( ["Blocker"]=4 ["Critical"]=10 ["Serious"]=20 ["Moderate"]=30 ) # Numbers in weeks | ||
|
||
# Required labels for filtering | ||
REQUIRED_LABELS=("A11y" "VPAT") | ||
|
||
# Updated SLA Labels | ||
SLA_LABELS=("SLA P1" "SLA P2" "SLA P3" "SLA Breach") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't have to do this now, but maybe a future enhancement would be to make these values configurable via inputs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will do this later one after running some successful runs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add a README.md, describing how this action is to be used? Check out the other action in this repo for an example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@manojvenkatap This comment still needs to be addressed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added readme.md, but i am not confident that the example provided will work or not because i have not tested. please suggest if any changes needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should be able to test it by following the instructions you provided in a test repo. You'll just point at the latest commit hash in this branch instead of main
.github/workflows/A11y SLA.yml
Outdated
env: | ||
GITHUB_TOKEN: ${{ secrets.PERSONAL_ACCESS_TOKEN }} | ||
run: | | ||
set -e |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I'm suggesting is you:
- Create a file called
label_a11y_issues.sh
- Make it executable (e.g.
chmod +x label_a11y_issues.sh
- Move everything that's in the
run
block into the file and add a#!/bin/bash
to the top of the file. Example:
#!/bin/bash
set -e
....
- Update this to just run the script:
run: ./label_a11y_issues.sh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly smaller things now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@manojvenkatap This comment still needs to be addressed
scripts/a11y-sla.sh
Outdated
# Determine impact level | ||
IMPACT_LEVEL="" | ||
for level in "${!LABEL_THRESHOLDS[@]}"; do | ||
if [[ "$LABELS" == *"$level"* ]]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need/want the *
? If we do any kind of loose checking, it seems like it should be to not be case sensitive. Something like:
if [[ "$LABELS" == *"$level"* ]]; then | |
if [[ "${LABELS,,}" == "${level,,}" ]]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Provided suggestion is matching if LABELS exactly equals Blocker, Critical, etc, But GitHub issues usually have multiple labels, and LABELS is like "A11y, VPAT, Serious", so LABELS != Serious exactly. so i changed to if echo "$LABELS" | grep -iq "$level"; then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, I misread this before. However, your example is still problematic if there is a label that contains one of these words (e.g. "very serious" or something).
A better approach would be to do something like this:
if echo "$issue" | jq -e '.labels[].name | ascii_downcase | select(. == (\"$level\" | ascii_downcase))' > /dev/null; then
This uses jq
to find if the label is in the list of labels by checking each label, rather than doing a string search against a concatenated string, which is potentially error prone
scripts/a11y-sla.sh
Outdated
REMOVE_LABELS=() | ||
|
||
for sla_label in "${SLA_LABELS[@]}"; do | ||
if [[ "$LABELS" == *"$sla_label"* ]]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above. Consider doing this:
if [[ "$LABELS" == *"$sla_label"* ]]; then | |
if [[ "${LABELS,,}" == "${sla_label,,}" ]]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See updated suggestion
the PR title should match our semantic commit convention: https://github.com/dequelabs/dev-process/blob/f981ec903da4f3f700b0d2af89014af84041e84b/git/README.md#commit-message-sections |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should be able to test it by following the instructions you provided in a test repo. You'll just point at the latest commit hash in this branch instead of main
scripts/a11y-sla.sh
Outdated
# Determine impact level | ||
IMPACT_LEVEL="" | ||
for level in "${!LABEL_THRESHOLDS[@]}"; do | ||
if [[ "$LABELS" == *"$level"* ]]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, I misread this before. However, your example is still problematic if there is a label that contains one of these words (e.g. "very serious" or something).
A better approach would be to do something like this:
if echo "$issue" | jq -e '.labels[].name | ascii_downcase | select(. == (\"$level\" | ascii_downcase))' > /dev/null; then
This uses jq
to find if the label is in the list of labels by checking each label, rather than doing a string search against a concatenated string, which is potentially error prone
scripts/a11y-sla.sh
Outdated
REMOVE_LABELS=() | ||
|
||
for sla_label in "${SLA_LABELS[@]}"; do | ||
if [[ "$LABELS" == *"$sla_label"* ]]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See updated suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is good enough that we can start using it. But in a follow-up PR, can you please:
- Add unit tests for a11y-sla.sh
- Add shellcheck
I can help you get this set up if you want to schedule a pairing session
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Since this file is only used in actions/a11y-sla-breach-labels, it would be best to collocate it there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we do this in next PR?
This GitHub workflow fetches accessibility tickets based on defined SLAs. If a ticket breaches the SLA, labels are added to track and prioritize issues for easier resolution.