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

Move around HCS schema and resource path definitions #1004

Merged
merged 1 commit into from
Apr 22, 2021

Conversation

dcantah
Copy link
Contributor

@dcantah dcantah commented Apr 21, 2021

As both the V1 and V2 HCS schema are tied to HCS itself, it makes more sense to have the schema definitions co-located under the hcs package. The resource paths are also tied to HCS and in preparation of some work where we'll need the resourcepaths available outside of the uvm and gcs packages, move the resource paths for both virtual machines and containers to their own package.

Signed-off-by: Daniel Canter dcanter@microsoft.com

@dcantah dcantah requested a review from a team as a code owner April 21, 2021 17:55
@kevpar
Copy link
Member

kevpar commented Apr 21, 2021

This seems like a good opportunity to prepare for clearer package organization in the future. These resource paths are all specific to the HCS/GCS schema and protocol, so I would like the package structure to reflect that.

Maybe a structure like:

internal/
    hcs/
        resourcepaths
        schema1
        schema2

Thoughts?

@dcantah
Copy link
Contributor Author

dcantah commented Apr 21, 2021

This seems like a good opportunity to prepare for clearer package organization in the future. These resource paths are all specific to the HCS/GCS schema and protocol, so I would like the package structure to reflect that.

Maybe a structure like:

internal/
    hcs/
        resourcepaths
        schema1
        schema2

Thoughts?

That looks great to me. I went back and forth on whether to add these to schema2 but those should ideally stay entirely generated. I'm in favor.

As both the V1 and V2 HCS schema are tied to HCS itself, it makes more sense
to have the schema definitions co-located under the hcs package. The resource paths
are also tied to HCS and in preparation of some work where we'll need the resourcepaths available
outside of the uvm and gcs packages, move the resource paths for both virtual machines and containers
to their own package.

Signed-off-by: Daniel Canter <dcanter@microsoft.com>
@dcantah dcantah changed the title Move virtual machine and container resource paths to new package Move around HCS schema and resource path definitions Apr 21, 2021
@dcantah
Copy link
Contributor Author

dcantah commented Apr 21, 2021

@kevpar Done

@dcantah
Copy link
Contributor Author

dcantah commented Apr 21, 2021

@ambarve @anmaxvl @msscotb I'd like everyone's thoughts on this. Looks pretty nice and makes more sense to me. I do need the virtual machine resource paths available outside of the uvm package regardless of the schema moving so if there's any objections I'll still need to plop those somewhere.

Copy link
Contributor

@katiewasnothere katiewasnothere left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@msscotb msscotb left a comment

Choose a reason for hiding this comment

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

looks good

Copy link
Member

@kevpar kevpar left a comment

Choose a reason for hiding this comment

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

LGTM

@dcantah dcantah merged commit b35557d into microsoft:master Apr 22, 2021
TBBle added a commit to TBBle/hcsshim that referenced this pull request Jul 14, 2021
This was left behind due to an unlucky conflict between microsoft#1004 and microsoft#930.

The file already existed with the same content at the new location, and
nothing was referencing this location, so a trivial deletion.

Signed-off-by: Paul "TBBle" Hampson <Paul.Hampson@Pobox.com>
princepereira pushed a commit to princepereira/hcsshim that referenced this pull request Aug 29, 2024
This was left behind due to an unlucky conflict between microsoft#1004 and microsoft#930.

The file already existed with the same content at the new location, and
nothing was referencing this location, so a trivial deletion.

Signed-off-by: Paul "TBBle" Hampson <Paul.Hampson@Pobox.com>
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.

4 participants