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

[DONE] add coveralls in pod-encoding action #1067

Merged
merged 34 commits into from
Mar 12, 2024
Merged

Conversation

ptitloup
Copy link
Contributor

@ptitloup ptitloup commented Mar 5, 2024

Before sending your pull request, make sure the following are done :

  • You have read our contribution guidelines.
  • Your PR targets the develop branch.
  • The title of your PR starts with [WIP] or [DONE].

@ptitloup ptitloup self-assigned this Mar 5, 2024
@ptitloup ptitloup changed the title [WIP] add coveralls in pod-encoding action [DONE] add coveralls in pod-encoding action Mar 11, 2024
Copy link
Contributor Author

@ptitloup ptitloup left a comment

Choose a reason for hiding this comment

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

Il manque pod_main.yml

@LoicBonavent LoicBonavent self-requested a review March 11, 2024 15:09
Copy link
Collaborator

@Badatos Badatos left a comment

Choose a reason for hiding this comment

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

quelques questions mineures, sinon code Ok pour moi ;)

DJANGO_SUPERUSER_PASSWORD: "passwd"
DJANGO_SUPERUSER_EMAIL: "noreplay@uni.fr"
ELASTICSEARCH_TAG: "elasticsearch:7.17.18"
ELASTICSEARCH_VERION: "elasticsearch:7.17.18"
Copy link
Collaborator

Choose a reason for hiding this comment

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

VERSIOn plutot que "VERION" non ? :)
Pourquoi avoir 2 variables (TAG et VERION) ?

"http://pod-back:8080/video/0001-video-test/",
"http://pod-back:8080/live/events/"
]
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

manque une ligne vide en fin de fichier

"http://pod-back:8080/video/0001-video-test/",
"http://pod-back:8080/live/events/"
]
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

manque une ligne vide en fin de fichier

COPY ./dockerfile-dev-with-volumes/pa11y-ci/my-entrypoint-pa11y.sh /tmp/my-entrypoint-pa11y.sh
RUN chmod 755 /tmp/my-entrypoint-pa11y.sh
# ENTRYPOINT ["pa11y-ci", "-c", "/usr/config.json"]
ENTRYPOINT ["bash", "/tmp/my-entrypoint-pa11y.sh"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

manque une ligne vide en fin de fichier

@@ -0,0 +1,3 @@
#!/bin/sh
echo "Launching commands into pa11y : pa11y-ci -c /usr/config.json"
sleep infinity
Copy link
Collaborator

Choose a reason for hiding this comment

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

manque une ligne vide en fin de fichier

@@ -0,0 +1,3 @@
#!/bin/sh
echo "Launching commands into pa11y : pa11y-ci -c /usr/config.json"
Copy link
Collaborator

Choose a reason for hiding this comment

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

pas d'espace avant les double points en anglais

n = 0
while self.video.encoding_in_progress:
print(
"... Transcripting in progress : %s " % self.video.get_encoding_step
Copy link
Collaborator

Choose a reason for hiding this comment

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

pas d'espace avant les double-points en anglais

@Badatos
Copy link
Collaborator

Badatos commented Mar 11, 2024

Attention, j'ai l'impression qu'en ayant retiré le "Check for pa11y failures" , s'il y a des erreurs A11y, elle seront transparentes et on ne les verra plus :/

- name: Check for pa11y failures.
         if: contains(steps.pa11y_output.outputs.content, 'Errors in http://')
         run: |
           echo "::error::The site is failing accessibility tests. Please review the comment in the pull request or the pa11y-ci step in the workflow for details."
           exit 1

Copy link
Collaborator

@Badatos Badatos left a comment

Choose a reason for hiding this comment

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

mettre à jour actions/setup-node@v3 en actions/setup-node@v4


- uses: actions/setup-node@v3
with:
node-version: 19
Copy link
Collaborator

Choose a reason for hiding this comment

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

Attention :

Node.js 16 actions are deprecated. Please update the following actions to use Node.js 20: actions/setup-node@v3. For more information see: https://github.blog/changelog/2023-09-22-github-actions-transitioning-from-node-16-to-node-20/.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

comprend pas, je peux laisser 19 ou c'est mieux de mettre 20 ?

@ptitloup
Copy link
Contributor Author

Attention, j'ai l'impression qu'en ayant retiré le "Check for pa11y failures" , s'il y a des erreurs A11y, elle seront transparentes et on ne les verra plus :/

- name: Check for pa11y failures.
         if: contains(steps.pa11y_output.outputs.content, 'Errors in http://')
         run: |
           echo "::error::The site is failing accessibility tests. Please review the comment in the pull request or the pa11y-ci step in the workflow for details."
           exit 1

Nope, s'il y a des erreurs, pa11y-ci sort avec le code 2 ce qui stop les tests

Copy link
Collaborator

@LoicBonavent LoicBonavent left a comment

Choose a reason for hiding this comment

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

Pour ma part, après les points remontés par Olivier et modifiés, cela me semble bon.
Après, c'est sûr qu'un max-complexity plus élevé serait top :)

@ptitloup ptitloup merged commit 93300ef into develop Mar 12, 2024
5 checks passed
@ptitloup ptitloup deleted the ptitloup/test_coveralls branch April 8, 2024 14:23
This pull request was closed.
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