-
Notifications
You must be signed in to change notification settings - Fork 0
feat: create github ci template for lenscore accessibility testing #24
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?
feat: create github ci template for lenscore accessibility testing #24
Conversation
4rokis
left a comment
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.
@Caknoooo Can you as part of the test for this template add it to the Access TIme Web V2 repo?
|
@Caknoooo SHould be a comment not request for changes |
This means that the one that was tried to scan etc. was AccessTime Web V2, right? |
.github/workflows/lens-core.yml
Outdated
| - name: Deploy with Vercel (if mode=vercel) | ||
| id: vercel_deploy | ||
| if: ${{ inputs.mode == 'vercel' }} | ||
| uses: amondnet/vercel-action@v25 |
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.
Is there official vercel action library ? let's try not use unofficial vercel action if possible.
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.
Okay ce. I'll explore official action
.github/workflows/lens-core.yml
Outdated
| description: 'Deployment mode: vercel | nextjs | custom' | ||
| required: true | ||
| type: string | ||
| url: | ||
| description: 'Target URL to scan (if already deployed/known)' | ||
| required: false | ||
| type: string | ||
| build-command: | ||
| description: 'Build command for nextjs/custom' | ||
| required: false | ||
| type: string | ||
| default: 'npm run build' | ||
| start-command: | ||
| description: 'Start command for nextjs/custom' | ||
| required: false | ||
| type: string | ||
| default: 'npm start' | ||
| port: | ||
| description: 'Local port the app listens to (nextjs/custom)' | ||
| required: false | ||
| type: number | ||
| default: 3000 | ||
| scan-depth: | ||
| description: 'Max crawl depth' | ||
| required: false | ||
| type: number | ||
| default: 2 | ||
| max-urls: | ||
| description: 'Max URLs to crawl' | ||
| required: false | ||
| type: number | ||
| default: 10 | ||
| timeout: | ||
| description: 'Request timeout (ms)' | ||
| required: false | ||
| type: number | ||
| default: 15000 | ||
| use-web-report: | ||
| description: 'Generate HTML report in addition to JSON' | ||
| required: false | ||
| type: boolean | ||
| default: true | ||
| fail-on-violations: | ||
| description: 'Fail CI when violations > 0' | ||
| required: false | ||
| type: boolean | ||
| default: true | ||
| vercel-project-path: | ||
| description: 'Relative path to the Vercel project (for monorepo)' | ||
| required: false | ||
| type: string | ||
| default: '.' | ||
| vercel-environment: | ||
| description: 'Vercel environment: preview | production' | ||
| required: false | ||
| type: string | ||
| default: 'preview' |
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.
@Caknoooo let's minimize the inputs and move it to GH Environment variables instead. It's quite bothersome to keep needing to change the settings if its put as an input (https://docs.github.com/en/actions/how-tos/write-workflows/choose-what-workflows-do/use-variables).
Let's leave mode and url only as input, as those are the one that potentially will need to be customized often, but don't use string as type, use choice (https://github.blog/changelog/2021-11-10-github-actions-input-types-for-manual-workflows/) so user can just choose.
e8af32f to
77d42b2
Compare
4rokis
left a comment
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.
Just a small think that I believe would make it so much cleaner.
@mneysa WDYT?
.github/workflows/lens-core.yml
Outdated
| workflow_call: | ||
| inputs: | ||
| mode: | ||
| description: 'Deployment mode (vercel | nextjs | custom)' |
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.
@Caknoooo This is more clear now and easier to read. However, I would go a step further and split it into 3 templates. vercel-lens-core-template.yml, nextjs-lens-core-template.yml , lens-core-template.yml. There will be a lot of shared and duplicated code but it will be much cleaner and easier for the final user to reuse.
As well place it to a diferent location as in this one it runs on lens-core as well 😅 Something like github-templates or something like that.
| type: string | ||
| secrets: | ||
| VERCEL_TOKEN: | ||
| required: false |
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 somehow make it that only the the required secret is a secret? Vercel topken should be a secret but org and project id should not
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.
Ohhh. That makes sense. I'll fix it
4rokis
left a comment
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.
SOme comments that need explanation
| import sys | ||
| import re | ||
| with open('/tmp/scan-output.txt', 'r') as f: |
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.
@Caknoooo Why is this needed? It reads the output and saves it into json? Why is this not done on the lens-core level?
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.
Ohhh sorry, I didn't notice this scan was still there. I'll fix it and try it on accesstime V2
README.md
Outdated
| url: '' | ||
| secrets: | ||
| VERCEL_TOKEN: ${{ secrets.VERCEL_TOKEN }} | ||
| VERCEL_ORG_ID: ${{ secrets.VERCEL_ORG_ID }} |
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.
@Caknoooo This is not updated
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.
Ohh my bad. I'll fix it
README.md
Outdated
|
|
||
| jobs: | ||
| accessibility: | ||
| uses: Access-Time/LensCore/.github/workflows/lens-core.yml@main |
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.
@Caknoooo Where does it takes this from? If I am adding it to my project. How does it know what Access-Time/LensCore is?
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 forgot to update the documentation. Maybe I'll fix and testing first and I'll update the documentation
|
@Caknoooo Can you add a PR to the AccessTIme web v2 with the vercel template? So we can see if and how it works? |
Sure, I'll try this |
|
@Caknoooo What's the status of adding it to web-v2? We need to see it in action :) |
Do you know how to invoke this CI on another repository? I've tried creating a pull request here, but the workflows aren't working. |
|
@Caknoooo There is still some issue with the template. Please look into it. Let's block this patch till we get a green light from the CI on the web |
| if [ "$SCAN_RESULTS" -eq 0 ]; then | ||
| echo "::warning::No pages were scanned. Check if the application is running and accessible." | ||
| fi | ||
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.
@Caknoooo We have a lot of set up here. Is there are way to extract it and share?
| # ------------------------- | ||
| # CHECK VIOLATIONS | ||
| # ------------------------- | ||
| - name: Check for violations |
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.
@Caknoooo Why do we need this? This seems too much custom logic. IMHO we should move this to the LensCore itself. We basically creating a text output no?
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.
@4rokis Yes, currently I am still in the research stage and it is not finished yet.
|
@Caknoooo What's the status? Is it ready for review? |
@4rokis Maybe can review now Using npm: https://github.com/vronnn/ts-nextjs-tailwind-travel/actions/runs/19369682617/job/55421966612 |
| fi | ||
| echo "✓ Created basic Dockerfile for $PM" | ||
| else | ||
| echo "✓ Using existing Dockerfile" |
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.
tbh, this is unnecessary, we should assume that user will have their own Dockerfile already, and if they not, then they should create one. We should not create template dockerfile for them because this most likely won't even work for their infrastructure and they will be forced to create their own Dockerfile.
I will just remove this and leave it with checking for Dockerfile file in the repo.
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.
Okey cee 👍
No description provided.