-
Notifications
You must be signed in to change notification settings - Fork 6
refactor(log-drain): move commands from v2 to v4 endpoints #187
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
Conversation
pdesoyres-cc
left a comment
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.
Thanks @Galimede for taking this subject.
- We should re-enable the drains e2e test suite which is skipped at the moment.
- You can run it locally with the right environment (I can help you create this environment if you want)
- Tests don't pass because the new v4 API is far from the v2 : input and output payloads are slightly different.
Do not hesitate to ping me if you need any help
d064049 to
862fe63
Compare
pdesoyres-cc
left a comment
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.
Thank you @Galimede for this PR. For a first one, it was quite tricky!
There are several things we need to fix. We can sync together or schedule a pair programming session if you need to.
src/clients/cc-api/commands/log-drain/create-log-drain-command.js
Outdated
Show resolved
Hide resolved
src/clients/cc-api/commands/log-drain/create-log-drain-command.js
Outdated
Show resolved
Hide resolved
src/clients/cc-api/commands/log-drain/create-log-drain-command.js
Outdated
Show resolved
Hide resolved
src/clients/cc-api/commands/log-drain/list-log-drain-command.types.d.ts
Outdated
Show resolved
Hide resolved
src/clients/cc-api/commands/log-drain/enable-log-drain-command.js
Outdated
Show resolved
Hide resolved
src/clients/cc-api/commands/log-drain/disable-log-drain-command.js
Outdated
Show resolved
Hide resolved
hsablonniere
left a comment
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.
First quick review, I commented some of @pdesoyres-cc's comments.
- Why did we delete the update tag command? What's the link with drains?
- Can we specify in the commit that it's a refactor of the new client or something like that?
- I wonder if we really need PascalCase draint target types, maybe we could directly use the upper snake case that the API needs in our types.
src/clients/cc-api/commands/log-drain/create-log-drain-command.js
Outdated
Show resolved
Hide resolved
src/clients/cc-api/commands/log-drain/delete-log-drain-command.js
Outdated
Show resolved
Hide resolved
|
+1 for > I wonder if we really need PascalCase draint target types, maybe we could directly use the upper snake case that the API needs in our types. |
017aafc to
8f83d4d
Compare
8f83d4d to
08f1b60
Compare
pdesoyres-cc
left a comment
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.
Almost there. Just some tiny things and it's all good
src/clients/cc-api/commands/log-drain/delete-log-drain-command.types.d.ts
Outdated
Show resolved
Hide resolved
b1d05cc to
ab8af5f
Compare
|
Note: I've had to keep the |
pdesoyres-cc
left a comment
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.
LGTM. well done @Galimede
Fixes #159
What does this PR do?
UpdateLogDrainCommandwith separateEnableLogDrainCommandandDisableLogDrainCommandlog-drain-utils.jswith polling utilities (waitForLogDrainEnabled,waitForLogDrainDisabled,waitForLogDrainDeletion)ApplicationIdtype (ownerId+applicationId)ListLogDrainCommand(status,executionStatus,executionStatusNotIn)UpdateTagCommand(cleanup)