Skip to content

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