Conversation
037db09 to
3c280d0
Compare
3c280d0 to
e2d990a
Compare
kurtmckee
left a comment
There was a problem hiding this comment.
Leaving feedback now, but I still need to review the test suite changes.
| :param aliases: List of target aliases to validate | ||
| :raises click.Abort: If any alias is not found | ||
| """ | ||
| config_aliases = {target.alias for target in context.config.targets} |
There was a problem hiding this comment.
Not a change request for this PR
This is the second time I'm seeing looping lookups of aliases due to the data type of the targets stored in the config (above, it's the next(<look for aliases that match>, None) line).
Since not every target will have an alias, I see that this isn't guaranteed to be the correct format, but our usage at publication time strongly suggests that we should be loading the targets to a dict.
There was a problem hiding this comment.
As it stands, the code currently enforces that every target has an alias and it's passed to the backend as the name. To make the required change, it would break the config format, but I do agree that it would be more efficient O(1) instead of O(n) to have the alias keyed from a dictionary, so:
Current structure:
{
"targets": [
{"alias": "create-foo", "path": "/foo", "method": "POST"},
{"alias": "get-bar", "path": "/bar", "method": "GET"}
]
} Your suggestion:
{
"targets": {
"create-foo": {"path": "/foo", "method": "POST"},
"get-bar": {"path": "/bar", "method": "GET"}
}
} There was a problem hiding this comment.
Since not every target will have an alias
Maybe I'm misunderstanding, but every target will have an alias (at least under the current config model: ref).
There was a problem hiding this comment.
Does that have a default value? If I haven't added the target yet then will it still have an alias?
There was a problem hiding this comment.
No default.
If you haven't added a target yet then it won't have an alias but also won't really "exist" in the sense that it will be recorded in our config.
I suppose that conceptually the idea of a "possible target" exists without an alias (i.e., an openapi path, method, and associated operation). But as far as gra is concerned, a "target" starts to exist once you "add it" to the config and a part of that process is providing a target-alias.
| _update_target(context, alias, target) | ||
|
|
||
| # Commit immediately after each successful publish | ||
| context.config.commit() |
There was a problem hiding this comment.
Not a request for changes in this PR
At some point in the future I'd like to create something (perhaps an at-exit handler) that commits just once as the program exits. Writing to disk over and over during program execution doesn't sit well with me! 😓
There was a problem hiding this comment.
There are some points, notably the manage command, where I think I disagree with removing in-execution commits. Open to being convinced but the pros of the "save-as-you-go" behavior are pretty high for a command like that so I'd want to understand the cons better.
There was a problem hiding this comment.
The cons are simply that I think it's bad practice to write files over and over. I'm actually not sure what the pros are to doing so in this context -- I only see the cons.
(For context: I've destroyed several dozen SD cards on Raspberry Pis and USB drives in a production environment due to write cycle limitations of the underlying media, so this is something I'm acutely aware of.)
There was a problem hiding this comment.
The main pro is that checkpointing makes it impossible to revert a bunch of changes that you thought you already made.
If you run:
gra manage
add target
add target
add target
modify target
repeat x5
then instead of exiting gracefully with the <exit> option, enter ctrl-c or have your console window closed for some reason, all of the changes you made over the last 10 - 30 min are persisted.
There's a secondary pro around displaying the config file as a "materialized version" as you edit it, but I think that's more "cool that you could do that" than impactful.
There was a problem hiding this comment.
As far as the "wrote-too-fast" concern goes,
The interaction model of gra manage is such that I wouldn't impose normal limitations on it that I might on "cli commands". Most cli commands don't stay active with back & forth interaction for 10s of minutes. Most cli commands do a thing (call an api, modify a file, compute a result) then terminate.
I agree with the concern around hot loops like
while True:
data = compute_data(...)
write_data(data)
But this is just a different interaction mode. It's more like:
while True:
command = solicit_command(...)
data = compute_data(command, ...)
write_data(data)
The human in the loop makes a big difference. They are a built-in rate limiter in your write rate.
Computing data may complete in nano-seconds or milliseconds but soliciting some feedback from a person will not.
| :param target: Target configuration | ||
| :return: Description string | ||
| """ | ||
| operation = context.manifest.registered_apis[alias].target.operation |
There was a problem hiding this comment.
What is the difference between target and context.manifest.registered_apis[alias].target?
There was a problem hiding this comment.
This is because two different target types are being used:
targetparameter:TargetConfigis from config.json and has method, path, and aliascontext.manifest.registered_apis[alias].targetis theOpenAPITargetfrom the manifest and has operation, destination
The confusing part is using target.method and target.path for fallback when we could use values from the manifest's destination.
I've decided to rename the target input to target_config and updated the docstrings. I'm currently working on a follow-up PR for the description b/c the description should actually be captured during the gra manage process when adding a target to the config. This get_description code will be removed in an upcoming PR and is currently serving as a stopgap solution [ref slack message].
There was a problem hiding this comment.
I'm still testing but this is the branch for the capture description during manage work: https://github.com/globus/globus-registered-api/compare/fix-capture-description-during-manage.
| ) | ||
|
|
||
| # Store the returned ID back in config | ||
| target.registered_api_id = UUID(response["id"]) |
There was a problem hiding this comment.
I see in the actual client that if no roles are defined then no roles are submitted (not even the user's account as a default).
Should the role information in the response be stored back into the roles, in addition to the ID?
There was a problem hiding this comment.
This is a good question, and I don't know the correct answer.
The current behavior is:
gra publish
- Reads
config.roles→ converts to URNs → sends to API - API returns: Full response including assigned roles
- We only store:
response["id"]→ back toTargetConfig.registered_api_id
@derek-globus, did you envision that roles would be stored back to the TargetConfig?
There was a problem hiding this comment.
My inclination would be no.
In the service, we auto-append the current user's identity as an owner during registered api creation.
So if I were deploying, for instance, the search service to flows with the starting config:
* targets
* ...
* roles
* group: search-admins
every registered api creation would add my individual user as an owner.
If we updated the server side to only ever attach callern_urn when they didn't already have some path to ownership (e.g., through a group), I'd be more onboard to treat that response as authoritative.
There was a problem hiding this comment.
I was asking because this is the create function.
I've observed that I could remove myself as the default owner using gra manage. However, if the service re-adds me as the owner of each target when I publish, I think that it will be beneficial to capture that by saving back the roles returned by the service (or, alternatively, perhaps notify the user that the values set during creation don't match the values returned by the service).
kurtmckee
left a comment
There was a problem hiding this comment.
I think that any remaining discussion on feedback I left isn't a blocker.
sc-48270
What
Add
publishCLI command to create and update registered APIs in the Flows service.Why
Users need to publish their configured API endpoints to the Flows service.
How
publishcommand incommands/publish/module with separate dispatch, domain, and publisher componentsregistered_api_idin config--target-aliasfiltering to publish specific targets