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

feat: Add project column to the images table and refactor ImageRef related logics #2707

Conversation

jopemachine
Copy link
Member

@jopemachine jopemachine commented Aug 13, 2024

Overview

  • Adds project column to the images table.
  • Refactors ImageRef to be a simple type that only holds data (dataclass).
    • Add project field. Previously, the name field mixed the project and image names, but now ImageRef.name only contains the image name, and the project value is separately in the ImageRef.project field.

Why

Saving the project value in the DB is necessary because the string corresponding to project may differ depending on the type of container repository, making it impossible to separate the project from the image canonical string.

The project value should also be included in the ImageRef field, and ImageRef should be refactored into a simple struct that only holds data, separated from the existing complex logic of parsing image strings.

Refactoring details

This PR performs relatively large-scale refactoring to add a project field to the ImageRef corresponding to the ImageRow.

ParsedImageStr type

  • ParsedImageStr is a namedtuple that represents the parsed result of an image string, containing properties such as canonical.
  • ParsedImageStr type is similar to the ImageRef type, but it cannot distinguish between the project and image name and does not include architecture information, so it does not correspond to a single ImageRow.
  • ImageRef.parse_image_str() returns a ParsedImageStr type, which can be used to create an ImageRef. But you can also create an ImageRef instance directly using ImageRef.from_image_str().

Comparison of ImageRef.parse_image_str() with the existing ImageRef() constructor

  • In the existing code, known_registries* which contains contextual information about registries, was separately provided and used to parse the image_str (value)
  • However, in this refactoring PR, ImageRef is now just a simple dataclass, so the parsing logic previously handled by the ImageRef constructor is now handled by ImageRef.parse_image_str().
  • ImageRef.parse_image_str() can only accept a single registry as an argument, unlike the existing code, which allows for multiple registries through known_registries.
  • The image string parsing logic still behaves the same as before. For example, the behavior when passing ['*'] to the known_registries parameter in the original code can be replicated by passing "*" to the registry argument in ImageRef.parse_image_str(). For more details, please refer to the comments in the ImageRef.parse_image_str() function and the test_docker.py test code.

ImageIdentifier type

  • A tuple of image canonical string, and architecture.
  • Uniquely corresponds one-to-one with an ImageRow.
  • ImageRow.resolve() can now be operated using ImageIdentifier, in addition to ImageAlias and ImageRef.

Add IMAGE_REF type to ExtTypes

  • Adds the ImageRef type to msgpack to enable the exchange of ImageRef type objects in RPC communication between the managers and agents.

Etcs


Checklist: (if applicable)

  • Milestone metadata specifying the target backport version
  • Migration script for filling the Image.project column
  • Test case(s) to:
    • Demonstrate the difference of before/after

📚 Documentation preview 📚: https://sorna--2707.org.readthedocs.build/en/2707/


📚 Documentation preview 📚: https://sorna-ko--2707.org.readthedocs.build/ko/2707/

@jopemachine jopemachine added this to the 24.09 milestone Aug 13, 2024
@jopemachine jopemachine changed the title feat: Add project column to the images table and refactoring ImageRef logic feat: Add project column to the images table and refactor ImageRef related logics Aug 13, 2024
Copy link
Member

@achimnol achimnol left a comment

Choose a reason for hiding this comment

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

  • Could we make ImageRef also a dataclass?
  • Could you elaborate why we need a separate ParsedImageStr?

@jopemachine
Copy link
Member Author

jopemachine commented Aug 14, 2024

@achimnol

  1. Could we make ImageRef also a dataclass?

I am currently working on refactoring Images to change the format so that ImageRef only holds decomposed values, although this has not yet been reflected in this draft PR.

  1. Could you elaborate why we need a separate ParsedImageStr?

ParsedImageStr is a type created to store the parsed form of the canonical image without the project for now, but I plan to remove it if it is no longer needed after the refactoring is completed.

@jopemachine jopemachine force-pushed the topic/08-13-feat_add_project_column_to_the_images_table_and_refactoring_imageref_logic branch 4 times, most recently from be60fc7 to 954ace4 Compare August 16, 2024 03:55
@jopemachine
Copy link
Member Author

We need to fix the CI that is currently broken after the PR below is merged.
#2643

@jopemachine jopemachine force-pushed the feat/migration-container-registries branch from d2da906 to 97f826c Compare August 20, 2024 03:46
@jopemachine jopemachine force-pushed the topic/08-13-feat_add_project_column_to_the_images_table_and_refactoring_imageref_logic branch from 9d94248 to 4425837 Compare August 20, 2024 03:46
@jopemachine jopemachine force-pushed the feat/migration-container-registries branch from 97f826c to 78477bc Compare August 21, 2024 07:35
@jopemachine jopemachine force-pushed the topic/08-13-feat_add_project_column_to_the_images_table_and_refactoring_imageref_logic branch from c04a2d1 to d042760 Compare August 21, 2024 07:36
@jopemachine jopemachine force-pushed the topic/08-13-feat_add_project_column_to_the_images_table_and_refactoring_imageref_logic branch from fa39d8d to b48fa28 Compare September 30, 2024 14:36
@kyujin-cho kyujin-cho added this pull request to the merge queue Sep 30, 2024
@jopemachine jopemachine modified the milestones: 24.03, 23.09, 24.09 Sep 30, 2024
Merged via the queue into main with commit 5decd9c Sep 30, 2024
24 checks passed
@kyujin-cho kyujin-cho deleted the topic/08-13-feat_add_project_column_to_the_images_table_and_refactoring_imageref_logic branch September 30, 2024 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:docs Documentations comp:agent Related to Agent component comp:cli Related to CLI component comp:common Related to Common component comp:manager Related to Manager component require:db-migration Automatically set when alembic migrations are added or updated size:XL 500~ LoC
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants