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

Confusing parameter #284

Closed
liangyuRain opened this issue Aug 13, 2018 · 4 comments
Closed

Confusing parameter #284

liangyuRain opened this issue Aug 13, 2018 · 4 comments

Comments

@liangyuRain
Copy link

liangyuRain commented Aug 13, 2018

In layer.go, the CreateSandboxLayer and CreateScratchLayer functions both have a parameter named parentId. It is a natural way to think that this is the parent layer of the layer we want to create, but from line 34 of create.go, we can deduce that the parentId is actually the base layer ID, because the order of paths to read-only parent layers given to wclayer create as argument should ends with base layer. Interestingly, the parentId parameter is ignored inside the CreateSandboxLayer and CreateScratchLayer functions, so it does not make any difference in the execution.

Here, I suggest probably we should delete that parameter. In addition, for argument specifying paths to parent layers like in create, import, export, and mount, we probably need a better documentation on the order and how the list should be given. The order should be ending with base layer instead of starting with it, and the list should have -l before every element like -l layer_3 -l layer_2 -l layer_1.

@lowenna
Copy link
Contributor

lowenna commented Aug 13, 2018

I agree we need (some) documentation - that hasn't been a focus.

Unfortunately, we can't change external APIs as there are multiple consumers, some of which we (Microsoft) don't control.

@jterry75
Copy link
Contributor

@jhowardmsft - Renaming a string param isn't a breaking change. I think if its just a rename we could accomplish fixing the confusing part without breaking the API. Thoughts?

@lowenna
Copy link
Contributor

lowenna commented Aug 14, 2018

Renaming is fine 😇 But it didn't sound like a rename unless I'm missing something?

@jterry75
Copy link
Contributor

No you are right. I was just saying we could call it baseLayerId to help with documentation when reading the code at least.

dcantah pushed a commit to dcantah/hcsshim that referenced this issue Mar 17, 2021
…_append

Fix additional append in host device list
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

No branches or pull requests

3 participants