making pubsub idempotent and other fixes #91
Conversation
pkg/infra/pulumi_aws/deploylib.ts
Outdated
| @@ -620,7 +620,7 @@ export class CloudCCLib { | |||
| publishers: string[], | |||
| subscribers: string[] | |||
| ): aws.sns.Topic { | |||
| let topic = `${this.name}_${path.replace(/[^0-9a-zA-Z_-]/g, '-')}_${varName}_${event}` | |||
| let topic = `${this.name}_${varName}_${event}` | |||
There was a problem hiding this comment.
Why remove the path? Is varName not the emitter's variable name anymore (ie, is it the ID)? Didn't this need to match with something in the runtime as well?
There was a problem hiding this comment.
so the path isnt in the runtime and i believe that varName is actually the ID and not truly the varName. Let me double check because if it is the varname we could have conflicts. I can also rename the variable to pubsubID so it makes more sense
pkg/lang/javascript/plugin_pubsub.go
Outdated
| @@ -360,7 +375,14 @@ func (p *Pubsub) generateEmitterDefinitions() (err error) { | |||
| } | |||
|
|
|||
| func findTopics(f *core.SourceFile, spec VarSpec, query string, methodName string) (topics []string) { | |||
| varName := findVarName(f, spec) | |||
| relPath, _ := filepath.Rel(filepath.Dir(f.Path()), spec.DefinedIn) | |||
There was a problem hiding this comment.
Error should be checked, even if it's to panic.
pkg/lang/javascript/plugin_pubsub.go
Outdated
| VarName: spec.VarName, | ||
| } | ||
|
|
||
| varName := findVarName(f, newSpec) |
There was a problem hiding this comment.
This seems like a shortcoming of findVarName that should be fixed inside, not outside.
There was a problem hiding this comment.
will move this logic in there and panic on the relPAth error. Otherwise we need a major refactor of finding every method that uses findVarName
There was a problem hiding this comment.
Realistically, relPath only errors when one of the paths is absolute, which we should never have. The panic is like an assert that makes sure that's true.
There was a problem hiding this comment.
It's been a while, so I forget exactly what all the inputs to findVarMean — but doesn't moving relPath, err := filepath.Rel(filepath.Dir(f.Path()), spec.DefinedIn) into it make it more language-specific? There are languages where relative imports aren't a thing.
There was a problem hiding this comment.
It is indeed language specific, but the var finder is in the javascript package so I'd expect it to be.
There was a problem hiding this comment.
🤦♂️ So it is. For some reason I thought it was in a language-agnostic package (and I didn't even think to double check that assumption in the path). Sorry — carry on! :)
There was a problem hiding this comment.
yeah i just realized, wouldnt this cause issues for golang?
There was a problem hiding this comment.
oh wait this is js specific
|
|
Are there tests (unit or integration) that we can add to prove the problem is fixed? |
• Does any part of it require special attention?
• Does it relate to or fix any issue? closes https://github.com/klothoplatform/klotho-pro/issues/26
Pubsub was not idempotent or able to find any modules not in the base path of klotho config. We also did not have the names lined up between what our infra launched and what code expected.
Needed to add proxy files at the end so the plugin doesnt reprocess them and see more subscribers/publishers than intended.
Need to make sure we arent overwriting our core resources.
Standard checks