Skip to content
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

Add codepath alerts #4141

Merged
merged 2 commits into from
Jan 25, 2025
Merged

Add codepath alerts #4141

merged 2 commits into from
Jan 25, 2025

Conversation

bretg
Copy link
Contributor

@bretg bretg commented Jan 8, 2025

Closes #4075

This is a port of the PR that was merged in the PBS-Java repo prebid/prebid-server-java#3645

the goal is to let bidders/modules register an email address so that when a PR is opened that affects one of their files, they're at least aware of it.

@bsardo bsardo self-assigned this Jan 15, 2025
bsardo
bsardo previously approved these changes Jan 23, 2025
@bsardo bsardo changed the title adding codepath alerts Adding codepath alerts Jan 23, 2025
Copy link
Contributor

@guscarreon guscarreon left a comment

Choose a reason for hiding this comment

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

I see the PBS-Java port got merged a couple of weeks ago, is it proven to work? Has there been an instance of an email been sent to one of the stakeholders yet?

.github/workflows/scripts/send-notification-on-change.js Outdated Show resolved Hide resolved
node-version: '18'

- name: Install dependencies
run: npm install axios nodemailer
Copy link
Contributor

Choose a reason for hiding this comment

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

fs and path are also listed as dependencies in .github/workflows/scripts/send-notification-on-change.js, do they need to get installed here? My javascript is a little rusty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does seem to be needed as it does work over in the Java repo

Copy link
Contributor

Choose a reason for hiding this comment

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

Does not seem to be needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

'fs' and 'path' do not need to be listed in the npm install for this to work.

@bretg
Copy link
Contributor Author

bretg commented Jan 24, 2025

Thanks Gus - yes, this works in the PBS-Java repo.

Copy link
Contributor

@guscarreon guscarreon left a comment

Choose a reason for hiding this comment

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

LGTM

@bsardo bsardo changed the title Adding codepath alerts Add codepath alerts Jan 25, 2025
@bsardo bsardo merged commit 1964dc7 into master Jan 25, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support "code owner" alerts
4 participants