search

Stedi-Demos / scheduled-sftp-poller

Invoke a Stedi Function to poll SFTP servers on a scheduled basis using GitHub actions
Apache License 2.0
1 stars 2 forks source link

First review feedback / ideas #2

Closed BDQ closed 1 year ago

BDQ commented 1 year ago

Didn’t know where to put the feedback for the ftp poller demo, so just dumping it all here:

Overall it was great, it worked first time - all of the above is just quality of life improvements.

rsioss commented 1 year ago
  • [ ] There’s a typo in the npm run configure-parners <-- missing t

good catch! I should have copied and pasted

  • [ ] Would you consider adding a quick Zod schema to validate the trading partner JSON? You could infer your runtime types from it too (we’re using it a lot in RealTruck to validate all sorts of structures, I think its a great pattern to catch translation/input issues).

I am actually using a quicktype schema which does the same. IMO it's even easier to get up and going compared to Zod. If you try with an invalid schema and run the configure-partners script you will see a validation error.

  • [ ] The top level items key is very generic and somewhat confusing, I’d expect something more like partners or profiles.

I used items to align with the Stash ListValuesCommand response. This way the same quicktype generated type can be used to validate the configuration and the Stash response. I can document the reasoning if you think it makes sense to do so.

  • [ ] The sample data we’re using like ANOTHERMERCH is very weak, I know you’re just copying from my earlier demos, but maybe we should pick a few nicer fake company names like Stark Inc or STARK01002 something that’s a bit more real looking and easier to relate too, maybe even MY_PARTNER_ISA ? Yeah, this is a good call -- I can update this across our demos. I wanted to keep it consistent, but agreed we should use a better name.

  • [ ] When I run the invoke-sftp-poller script, there’s no output - maybe sprinkle in a few console.logs - I had no clue where the data was going - and I ran it on an SFTP with 20+ files, so it just sat there . I CRTL+C after a few minutes, so maybe I missed the final output.

Ahh, yep, I bet you just cancelled too early. I will sprinkle in some output messages.

  • [ ] I think you’ve got a prefixed / when you’re writing to the target bucket, I had a folder with just / as the name and inside that I had the trading-partners directory. See screenshot. image

I noticed this yesterday as well -- I thought that the PutObjectCommand key supported both relative and absolute paths (and it actually seems to, because the object gets uploaded successfully). I'm wondering if this is a Buckets UI issue? Interestingly, when uploaded with the preceding / the bucket notification comes in as expected and gets processed by the read EDI handler that I have deployed. Maybe it's getting a notification with a double leading / though. Will check.

  • [ ] I’d expect to be able to configure where the data is going, dumping onto the default sftp bucket feels a touch dangerous, even for a demo. Maybe spin up a dedicated bucket? And/or allow supplying a base path where all files get dropped, it could default to trading-partners - and maybe explain what file structure the files will be placed into by the script in the README?

I can definitely make it configurable. I think putting it in the default sftp bucket is actually a good idea -- the input files can be processed in the same manner as input files that are received by partners using Stedi SFTP. I would be interested in hearing more about what feels dangerous to you with this approach.

  • [ ] I had a few niggles with the README text too, I can submit a PR for that, or better, get our man in the kingdom by the sea to do a review.

A PR would be welcome! I did ping Joost yesterday for his feedback, but would be happy to get your feedback incorporated too.

Overall it was great, it worked first time - all of the above is just quality of life improvements.

🙏 thank you for giving it a run through! The feedback is greatly appreciated

BDQ commented 1 year ago

I am actually using a quicktype schema which does the same. IMO it's even easier to get up and going compared to Zod. If you try with an invalid schema and run the configure-partners script you will see a validation error.

I missed spelled externalSftpConfig to externalSftpConfigs and it created the value in the Keyspace without issue, so I guess the validation isn't run by the configure-parners script?

I used items to align with the Stash ListValuesCommand response. This way the same quicktype generated type can be used to validate the configuration and the Stash response. I can document the reasoning if you think it makes sense to do so.

That's nice for your development experience, but from an approachability perspective I think a more descriptive name would help.

I can definitely make it configurable. I think putting it in the default sftp bucket is actually a good idea -- the input files can be processed in the same manner as input files that are received by partners using Stedi SFTP. I would be interested in hearing more about what feels dangerous to you with this approach.

Maybe if you explicitly call it out in the docs, I just felt blind as to where the files were going to go, it feels like it's too magical - I prefer complete visibility/control.

rsioss commented 1 year ago

I missed spelled externalSftpConfig to externalSftpConfigs and it created the value in the Keyspace without issue, so I guess the validation isn't run by the configure-parners script?

ahh, interesting -- it does do the validation, but because externalSftpConfig is optional it won't fail if it's missing. does Zod throw an error for extra properties being present?

That's nice for your development experience, but from an approachability perspective I think a more descriptive name would help.

Sure, I can use separate types for the configuration and the Stash response

Maybe if you explicitly call it out in the docs, I just felt blind as to where the files were going to go, it feels like it's too magical - I prefer complete visibility/control.

yeah, I Think it makes sense to both call it out in the docs and to make it configurable

rsioss commented 1 year ago

Maybe if you explicitly call it out in the docs, I just felt blind as to where the files were going to go, it feels like it's too magical - I prefer complete visibility/control.

yeah, I Think it makes sense to both call it out in the docs and to make it configurable

Following up on this: I can really only add a console.log to indicate that the function is being invoked (which I've done). I can't print anything to the console from the function itself though, as it's running in the Stedi cloud / Lambda environment.

rsioss commented 1 year ago

I can definitely make it configurable. I think putting it in the default sftp bucket is actually a good idea -- the input files can be processed in the same manner as input files that are received by partners using Stedi SFTP. I would be interested in hearing more about what feels dangerous to you with this approach.

Maybe if you explicitly call it out in the docs, I just felt blind as to where the files were going to go, it feels like it's too magical - I prefer complete visibility/control.

I ended up adding this to the trading partner profile schema... configurable per partner now!:

  bucketConfig?:       BucketConfig        // optional path prefixes used for reading/writing files for this trading partner