Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

replace fs implementation with v3 client #70

Merged
merged 4 commits into from
Jan 13, 2023
Merged

replace fs implementation with v3 client #70

merged 4 commits into from
Jan 13, 2023

Conversation

jhsinger-klotho
Copy link
Contributor

@jhsinger-klotho jhsinger-klotho commented Jan 11, 2023

• Does any part of it require special attention?
• Does it relate to or fix any issue? closes https://github.com/klothoplatform/klotho-history/issues/528

Changing the fs implementation in node so we dont use the large aws sdk. also added delete object as unlink

Tested by using the cloudfs sample app and running integration tests

Standard checks

  • Unit tests: Any special considerations? no
  • Docs: Do we need to update any docs, internal or public? upon approval will add delete to docs in node api
  • Backwards compatibility: Will this break existing apps? If so, what would be the extra work required to keep them working? yes

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a few questions as I looked at the added lines, but then when I looked at the removed lines, it looks like this is pretty equivalent to what was there before. So, treat all of my comments as non-blocking — my approval is intended. :)

})

async function getCallParameters(paramKey, dispatcherMode) {
let isEmitter = dispatcherMode === 'emitter' ? true : false
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

Suggested change
let isEmitter = dispatcherMode === 'emitter' ? true : false
let isEmitter = dispatcherMode === 'emitter'

if (isEmitter && Array.isArray(parameters)) {
// Emitters only have 1 parameter - the runtime saves an array, so we
// normalize the parameter
parameters = _.get(parameters, '[0]')
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this different from just parameters[0]? I tested, and it looks like parameters[0] evaluates to undefined if the array is empty, so we don't need index-bounds protection.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a lot of this was copy paste from the old implementation, going to try to remove some of the more odd things like this

if (Array.isArray(parameters)) {
let paramPairs = _.toPairs(parameters)
paramPairs = paramPairs.map((x) => {
if (_.get(x, '[1].type') == 'Buffer') {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similarly, if this is for index bounds, I think typescript can do it as just if (x[1]?.type == 'Buffer')


return parameters || {}
} catch (e) {
console.error(e)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't we want to rethrow? (Ditto for similar lines below)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good call, we should ill add that in, was my miss

}
try {
await s3Client.send(new PutObjectCommand(bucketParams))
console.log('Successfully uploaded object: ' + bucketParams.Bucket + '/' + bucketParams.Key)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this should be debug? I'd be worried about spamming the service logs with what is essentially an implementation detail (they should be able to assume that if the s3 didn't log a failure, that it succeeded).

Similar for other log lines, other than the ones within catches.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm, yeah i can add them all as debug

if (Array.isArray(parameters)) {
let paramPairs = _.toPairs(parameters)
paramPairs = paramPairs.map((x) => {
if (x[1].type == 'Buffer') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (x[1].type == 'Buffer') {
if (x[1] instanceof Buffer) {

Copy link
Contributor

@gordon-klotho gordon-klotho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 last usage of lodash we can remove

let paramPairs = _.toPairs(parameters)
paramPairs = paramPairs.map((x) => {
if (x[1].type == 'Buffer') {
return [x[0], Buffer.from(x[1].data)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this supposed to be a clone then? It's already a buffer, so this just reads it into another buffer, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like it, i can just change it to return x[1].data

// normalize the parameter
parameters = parameters[0]
if (Array.isArray(parameters)) {
let paramPairs = _.toPairs(parameters)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Object.entries I believe is the equivalent to toPairs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@github-actions
Copy link

Package Line Rate Health
github.com/klothoplatform/klotho/pkg/analytics 2%
github.com/klothoplatform/klotho/pkg/annotation 24%
github.com/klothoplatform/klotho/pkg/core 20%
github.com/klothoplatform/klotho/pkg/env_var 82%
github.com/klothoplatform/klotho/pkg/exec_unit 45%
github.com/klothoplatform/klotho/pkg/infra/kubernetes 58%
github.com/klothoplatform/klotho/pkg/infra/kubernetes/helm 52%
github.com/klothoplatform/klotho/pkg/input 63%
github.com/klothoplatform/klotho/pkg/lang 37%
github.com/klothoplatform/klotho/pkg/lang/dockerfile 0%
github.com/klothoplatform/klotho/pkg/lang/golang 9%
github.com/klothoplatform/klotho/pkg/lang/javascript 47%
github.com/klothoplatform/klotho/pkg/lang/python 60%
github.com/klothoplatform/klotho/pkg/lang/yaml 0%
github.com/klothoplatform/klotho/pkg/logging 7%
github.com/klothoplatform/klotho/pkg/multierr 95%
github.com/klothoplatform/klotho/pkg/provider/aws 60%
github.com/klothoplatform/klotho/pkg/runtime 75%
github.com/klothoplatform/klotho/pkg/static_unit 32%
github.com/klothoplatform/klotho/pkg/validation 73%
Summary 42% (3592 / 8590)

@jhsinger-klotho jhsinger-klotho merged commit 2f169fd into main Jan 13, 2023
@jhsinger-klotho jhsinger-klotho deleted the fs_v3 branch January 13, 2023 20:07
atorres-klo added a commit that referenced this pull request Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants