Skip to content

Conversation

@PauwelsPieter
Copy link
Contributor

@PauwelsPieter PauwelsPieter commented Jan 22, 2025

Summary

What's changed?
Included the possibility to configure NATS triggers.

Why do we need this?
#254

How have you tested it?
Creating and deleting a nats trigger.

Checklist

  • I have reviewed this myself
  • There is a unit test covering every change in this PR
  • I have updated the relevant documentation

Details

@cyclimse
Copy link
Contributor

cyclimse commented Feb 5, 2025

LGTM!

Thanks a lot for the contribution!

There are some formatting errors can you run npm run format?

I think we should also add SQS triggers afterward, but we can do it on our side.

Happy to do a release for this.

@PauwelsPieter
Copy link
Contributor Author

LGTM!

Thanks a lot for the contribution!

There are some formatting errors can you run npm run format?

I think we should also add SQS triggers afterward, but we can do it on our side.

Happy to do a release for this.

Hi there!

Sorry for the late reply. I didn't see the message.

I've run Prettier, so it should be all set! Thank you for picking this up!

@PauwelsPieter
Copy link
Contributor Author

LGTM!
Thanks a lot for the contribution!
There are some formatting errors can you run npm run format?
I think we should also add SQS triggers afterward, but we can do it on our side.
Happy to do a release for this.

Hi there!

Sorry for the late reply. I didn't see the message.

I've run Prettier, so it should be all set! Thank you for picking this up!

@PauwelsPieter
Copy link
Contributor Author

@cyclimse The pipeline is failing due to authentication is denied. Is there anything I can assist with in order to get it working and this PR merged?

@cyclimse
Copy link
Contributor

Arf, I'm really sorry. We don't have a lot of bandwidth, and have been sitting on it. We've been mostly waiting for someone from the team to test it manually. I'm "doctor" this week so I can take a look.

Copy link
Contributor

@cyclimse cyclimse left a comment

Choose a reason for hiding this comment

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

Sorry for the delay.

LGTM! I tested it and it worked great.

I've left some small comments but they're mostly kludges. I'm just waiting for the go ahead from the team to merge it.

Thanks a lot for the contribution.

key: value
key2: value2
- nats:
name: my-nats-event
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I think it's possible to include this directly in the top-level message, since all triggers can be given a name:

events:
 - name: my-nats-event
   nats:
     scw_nats_config:
       ...
     

However, it's okay like this, the API doesn't define a clear standard for what's a trigger and I don't reckon it's a big deal.

@@ -1,6 +1,6 @@
# Events
Copy link
Contributor

Choose a reason for hiding this comment

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

praise: thanks for updating the docs, really appreciated 🙏

@@ -0,0 +1,8370 @@
lockfileVersion: "9.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: haha I use pnpm as well, but in this case it's better to avoid comitting the lock IMO, since this codebase is supposed to be used with npm

@cyclimse cyclimse merged commit dc8e060 into scaleway:master May 21, 2025
18 of 44 checks passed
@cyclimse cyclimse mentioned this pull request May 21, 2025
3 tasks
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