- 
                Notifications
    
You must be signed in to change notification settings  - Fork 89
 
bump aws sdk v2 to v3 #2254
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
bump aws sdk v2 to v3 #2254
Conversation
          Hello sylvainsenechal,My role is to assist you with the merge of this Available options
 Available commands
 Status report is not available.  | 
    
d8b16cc    to
    6c6728b      
    Compare
  
    fea7c95    to
    e60add3      
    Compare
  
    
          Waiting for approvalThe following approvals are needed before I can proceed with the merge: 
  | 
    
e1dc391    to
    9ba51a2      
    Compare
  
    bcdd21f    to
    e791efe      
    Compare
  
    667a273    to
    54ba63d      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Congrats for the work done. Not so easy to switch from callback to async/await and the code is clean and I didn't detect some logic issue. 👏
For eslint / or syntax I didn't add a comment on all places where is it, but if you fix one, check other occurences
        
          
                tests/zenko_tests/node_tests/smoke_tests/vault_admin_and_IAM_API_tests/iamApi.js
          
            Show resolved
            Hide resolved
        
              
          
                tests/zenko_tests/node_tests/smoke_tests/vault_admin_and_IAM_API_tests/adminApi.js
          
            Show resolved
            Hide resolved
        
      | { Bucket: bucket, Key: key }, | ||
| )); | ||
| const chunks = []; | ||
| // eslint-disable-next-line no-restricted-syntax | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, a comment to explain why you don't follow our linter and/or change it to make it compliant with it 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UP
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say this is not really useful, as the goal is to remove all these files and only use CTST
        
          
                tests/zenko_tests/node_tests/iam_policies/cloudserver/AssumeRole.js
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                tests/zenko_tests/node_tests/iam_policies/cloudserver/AssumeRole.js
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                tests/zenko_tests/node_tests/cloudserver/keyFormatVersion/tests/versioningSuspended.js
          
            Show resolved
            Hide resolved
        
              
          
                tests/zenko_tests/node_tests/cloudserver/keyFormatVersion/tests/versioningSuspended.js
              
                Outdated
          
            Show resolved
            Hide resolved
        
      
          Waiting for approvalThe following approvals are needed before I can proceed with the merge: 
 The following reviewers are expecting changes from the author, or must review again:  | 
    
8ddae12    to
    55f651a      
    Compare
  
    | }, () => cb(err)); | ||
| })).then(() => cb(err)).catch(abortErr => { | ||
| const aggregateError = new Error(`Original error: ${err}; Abort failed: ${abortErr}`); | ||
| aggregateError.originalError = err; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we do that now ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mhh, just a way to keep track of both error, not the cleanest but also not super important as its a test.
Here, we keep track of both the error from completeMultiPartUpload, and the error from abortMultiPartUpload
        
          
                tests/zenko_tests/node_tests/iam_policies/cloudserver/AssumeRole.js
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                tests/zenko_tests/node_tests/iam_policies/cloudserver/AssumeRole.js
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                tests/zenko_tests/node_tests/iam_policies/cloudserver/AssumeRole.js
              
                Outdated
          
            Show resolved
            Hide resolved
        
      
          Waiting for approvalThe following approvals are needed before I can proceed with the merge: 
  | 
    
| next => sourceClient.getObject(versionId, err => { | ||
| assert.strictEqual(err.code, 'InvalidObjectState'); | ||
| assert.strictEqual(err.statusCode, 403); | ||
| const errorCode = err.Code || err.code; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we know in advance what the code field will be? or is it to differentiate between different error types (but should be the same, as we use the same client?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I can, I'll clean this
55f651a    to
    ef5f304      
    Compare
  
    | 
           /approve  | 
    
          In the queueThe changeset has received all authorizations and has been added to the The changeset will be merged in: 
 The following branches will NOT be impacted: 
 There is no action required on your side. You will be notified here once IMPORTANT Please do not attempt to modify this pull request. 
 If you need this pull request to be removed from the queue, please contact a The following options are set: approve  | 
    
          Queue build failedThe corresponding build for the queue failed: 
 Remove the pull request from the queue
  | 
    
| 
          
 I have successfully merged the changeset of this pull request 
 The following branches have NOT changed: 
 Please check the status of the associated issue ZENKO-5054. Goodbye sylvainsenechal.  | 
    
Issue: ZENKO-5054