-
-
Notifications
You must be signed in to change notification settings - Fork 63
Conversation
I added new test for the 403-scenarios. I think its mergeable then (I'd say as breaking change) |
providers: [KeyspaceService], | ||
exports: [KeyspaceService], | ||
}) | ||
export class AstraApiModule {} |
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.
Maybe this could be called Astra
to match the folder structure, then if it clashes in the imports we could use an alias
e.g.
import { AstraService as AstraApiService } from '../astra/astra.service';
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.
AstraModule
already gets imported from my package, that's why I named it AstraApi
but I think the alias should be no problem
// canActivate(ctx: ExecutionContext) { | ||
// return super.canActivate(ctx); | ||
// } | ||
// } |
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.
Is this still needed?
This looks amazing!! A few tiny comments. But really great work 💪 💥 |
@UseGuards(TokenGuard) | ||
@ApiSecurity('token') | ||
@ApiQuery({ name: 'keyspace', required: true }) | ||
getTokens(@Query('keyspace') keyspace: string) { |
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 think this should be part of the path. I can change it in my PR
required: true, | ||
}) | ||
@HttpCode(204) | ||
deleteClient(@Query() query) { |
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 think this should be part of the path, I can change it in my PR. What do you think?
Is there something missing if we make the mentioned changes in the other PR? |
I think this is ready to go? any improvements we can make in the next PR |
I created the PR, so I cannot approve 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.
Looks great 💥
I'd merge it as a breaking change.
@eddiejaoude |
Oh good spot 👍 thank you, I will do it after dinner |
Creating the secret now on Kubernetes, but naming it |
closes #137