RSDK-9286 - simplify org-location-machine-part hierarchy #4703
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
viambot
added
the
safe to test
stuqdog
commented
Jan 13, 2025
| @@ -1327,7 +1327,7 @@ var app = &cli.App{ | |||
| Usage: "removes binary data with file IDs in a single org and location from dataset", | |||
| UsageText: createUsageText("dataset data remove", | |||
| []string{datasetFlagDatasetID, generalFlagOrgID, dataFlagLocationID, dataFlagFileIDs}, false), | |||
| // TODO(RSDK-9286) do we need to ask for og and location here? | |||
| // CR erodkin: we need these because currently the way the Data APIs work, if you don't pass the primary org ID then it won't work correctly and I don't think we can properly infer which one is the "correct" one. We could do something hacky! but probably better to leave this to data to fix, and flag this to reviewers in the meantime. | |||
| Flags: []cli.Flag{ | |||
There was a problem hiding this comment.
We continue to ask for and require all of org/location/machine for RemoveDataFromDataset. Per discussion with @dmhilly and @tahiyasalam, the behavior here will change depending on which org ID is passed, and if we pass a secondary org ID instead of the primary owning org ID then data will not be removed correctly.
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
purplenicole730
requested changes
Jan 14, 2025
cli/app.go
Outdated
| @@ -1727,8 +1724,14 @@ var app = &cli.App{ | |||
| Name: "status", | |||
| Usage: "display part status", | |||
| UsageText: createUsageText("machines part status", []string{generalFlagMachine, generalFlagPart}, true), | |||
There was a problem hiding this comment.
if machine is no longer required, this has to be updated. same goes for all the other commands
cli/app.go
Outdated
| @@ -1888,10 +1877,14 @@ var app = &cli.App{ | |||
| Usage: "start a shell on a machine part", | |||
| Description: `In order to use the shell command, the machine must have a valid shell type service.`, | |||
| UsageText: createUsageText( | |||
| "machines part shell", []string{generalFlagOrganization, generalFlagLocation, generalFlagMachine, generalFlagPart}, false, | |||
| "machines part shell", []string{generalFlagMachine, generalFlagPart}, false, | |||
There was a problem hiding this comment.
machine doesn't seem to be required
viambot
added
safe to test
purplenicole730
approved these changes
Jan 16, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Minimizes the information needed when working with the org/location/machine/part hierarchy. We continue to allow for all information to be passed and indicate as such because the way information is passed alters what's necessary, i.e. passing just part is okay if we pass the part ID, but not if we pass the part name.
As a flyby, updates the way the hierarchy is shared to ensure accuracy. Previously, if we passed just a machine ID and a part ID, we'd list the org and location as the alphabetically first ones even when that wasn't correct.
Tested locally by running commands with minimized information, ensured no change in behavior.