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

Add inginious-task-tester workflow template #1

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

nrybowski
Copy link
Member

@nrybowski nrybowski commented Sep 8, 2022

Steps

  • Cache
    • Pip packages
    • APT packages
    • INGInious and INGInious-containers repositories
    • INGInious docker images
  • Installation and usage of additional task types from plugins
  • Remove TODOs when patches are merged in main INGInious repo
    • Use main repo when patches are merged
    • Remove default_install patch checkout
    • Remove task_test patch checkout
    • Remove latest task_test patch pull

Todo in new feature requests

@nrybowski nrybowski marked this pull request as ready for review November 28, 2022 18:10
@nrybowski
Copy link
Member Author

@anthonygego This PR is ready for review.


containers = os.environ.get('REQUIRED', 'base').split(' ')
container_names = []
for container in containers:
Copy link
Member

Choose a reason for hiding this comment

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

Here containers are iteratively built based on what the environment file contains. A choice has to made be made, either to:

  • document and tell the users to specify the environments in order such that dependencies are met, otherwise it might behave strangely, crashing or pulling older versions on DockerHub.
  • introspect dockerfiles recursively as it is done with inginious-install to solve dependencies automatically.

I'm OK with choosing the first option. The second can also be implemented later, as this is not blocking.

In the second case it would be nice to refactor the base code and allow to use it outside of inginious-install. This way, in case we add support for external Dockerfiles some day, those scripts would be ready too.

Copy link
Member Author

Choose a reason for hiding this comment

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

As we discussed some time ago, the best solution would be building and pushing INGInious containers automatically on a registry (e.g. DockerHub and/or GitHub.). We could then simply pull the images from there rather than rebuilding them when the cache is not filled. This would also ensure uniformity between all the CI instances.

I will open a Feature Request on the main repo for that.

Copy link
Member

Choose a reason for hiding this comment

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

Should I consider this as a choice for option 1 and that the PR is ready for merge then ?

The shipped version of python within the GitHub Workflow VM (Ubuntu20.04)
does not support typed syntax such as 'tule[int, list]'. Hence, the
explicit installation of a newwer python version is required.
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