-
Notifications
You must be signed in to change notification settings - Fork 6
feat: use new config model #357
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
This PR will trigger a minor release when merged. |
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 good so far!
You'd need to update the API spec as well, around the site config/org config.
@@ -53,7 +51,7 @@ async function getSitesToAudit(dataAccess, url, deliveryType, orgs) { | |||
const site = await dataAccess.getSiteByBaseURL(url); | |||
sitesToAudit = site ? [site] : []; | |||
} | |||
return sitesToAudit.filter((site) => !isAuditsDisabled(site, orgs[site.getOrganizationId()])); | |||
return sitesToAudit; |
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.
The js doc needs to be updated, because in the new version we don't have a flag to disable all handler types for a site.
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.
Nice!
alerts: | ||
- type: '404' | ||
byOrg: true | ||
handlers: |
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.
We can start reusing examples, when they become repetitive: https://swagger.io/docs/specification/adding-examples/ -> Reusing Examples
docs/openapi/schemas.yaml
Outdated
description: The Slack mentions to include in the alert | ||
$ref: '#/SlackMentionConfig' | ||
excludedURLs: | ||
description: URLs to be excluded from the alert |
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.
nitpicking: URLs to be excluded from the handler's processing?
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.
Would manualOverwrites
, fixedURLs
from broken-backlinks need to be added here as well or would they be allowed under broken-backlinks
key?
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 will add them as well
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.
@alinarublea would this cover the manualOverwrites
doc from #387 ? Or can I update the PR after this one is merged?
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.
Yes, it s exactly that, I wont modify it then and I will let you know after I merge
🎉 This issue has been resolved in version 1.56.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Please ensure your pull request adheres to the following guidelines:
describe here the problem you're solving.
If the PR is changing the API specification:
yet. Ideally, return a 501 status code with a message explaining the feature is not implemented yet.
If the PR is changing the API implementation or an entity exposed through the API:
Related Issues
Thanks for contributing!