Add collection flag for postman ECS agent #234
Conversation
gmann42
commented
Sep 15, 2023
gmann42
commented
- Adds collection flag
cmd/internal/ecs/ecs.go
Outdated
| } | ||
| return cmderr.AkitaErr{ | ||
| Err: fmt.Errorf( | ||
| "could not find the serviceId for %q", |
There was a problem hiding this comment.
This doesn't mean anything to the user; try to avoid showing "serviceID". There are three cases to consider:
- The collection ID doesn't exist.
- The API key presented doesn't have access (or I suppose the API key is absent?)
- There was an internal problem creating the new service and they need support.
I don't think we can distinguish between 1 + 2, but we should have a friendly error message specifying that the problem is almost certainly their key. (And if they didn't specify one at all, make clear that that is the problem!)
There was a problem hiding this comment.
Yeah I was thinking of discussing the same, Once I have the end to end things working will fix this.
There was a problem hiding this comment.
The correct error states handling is being already done by the function GetOrCreateServiceIDByPostmanCollectionID
cmd/internal/ecs/add.go
Outdated
| {Name: aws.String("POSTMAN_AWS_REGION"), Value: &wf.awsRegion}, | ||
| {Name: aws.String("POSTMAN_ECS_SERVICE"), Value: &wf.ecsService}, | ||
| {Name: aws.String("POSTMAN_ECS_TASK"), Value: &wf.ecsTaskDefinitionFamily}, |
There was a problem hiding this comment.
If these are modified, then the code that reads them back out should also be modified, ideally in a backwards-compatible way. As we discussed, there is some question about their value because tags aren't visible in the Postman UI at all (although they could be -- for example we might start annotating each collection with information about the sources of data.) So I would be OK leaving these as AKITA_ for now, but if they change they should change both places.
|
It looks like "Akita" still appears a bunch of times in prompts or log messages, I assume that is TBD in OBS-365? |
Yeah I tried removing it wherever I saw it. Will scan more closely as a part of OBS-365 |
