-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: slack client to support multiple org #102
Conversation
This PR will trigger a minor release when merged. |
# Conflicts: # package-lock.json
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'd propose to split the client-factory part from the actual slack client. this aligns with the principle of single responsibility, ensuring that each client instance is dedicated to a specific target (i.e., a Slack workspace). This redesign would simplify the client interface by removing the need to specify the target for every public method call.
Incomplete example:
class SlackClientFactory {
constructor(log) {
this.clients = {};
this.log = log;
}
// createFrom / lambda context handling here...
getClientForTarget(target) {
if (!this.clients[target]) {
const token = this.getTokenForTarget(target);
if (!token) {
throw new Error(`No token found for target: ${target}`);
}
this.clients[target] = new SlackClient(token, this.log);
}
return this.clients[target];
}
getTokenForTarget(target) {
return process.env[`${ENV_PREFIX}${target}`];
}
}
class SlackClient {
constructor(token, log) {
this.client = new WebClient(token);
this.log = log;
}
async postMessage(message, opts = {}) {
try {
const result = await this.client.chat.postMessage({
...DEFAULT_PARAMS,
text: message,
...opts,
});
return {
channelId: result.channel,
threadId: result.ts,
};
} catch (error) {
this.log.error('Slack message failed to send');
throw error;
}
}
}
this also benefits security:
- a client only can deal with one workspace and not accidentally others
- it is ground work for later adding roles, i.e. clients backed by different scoped slack tokens, for example a privileged client that can create channels, while regular clients only send messages, etc.
I incorporated all the reviews and added the file upload method. Please re-review |
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, only minor comments
# Conflicts: # package-lock.json
🎉 This PR is included in version @adobe/spacecat-shared-slack-client-v1.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Please ensure your pull request adheres to the following guidelines:
Related Issues
Thanks for contributing!