Skip to content

ref(core): Refactor options validation#104

Merged
Lms24 merged 4 commits into
mainfrom
lms-validate-options
Nov 11, 2022
Merged

ref(core): Refactor options validation#104
Lms24 merged 4 commits into
mainfrom
lms-validate-options

Conversation

@Lms24
Copy link
Copy Markdown
Member

@Lms24 Lms24 commented Nov 10, 2022

This PR refactors the validation of the plugin options. Since Sentry CLI does most of the validation work out of the box, we only need to take care of a few special cases. Therefore:

  • Deleted the ugly and duplicated org, project, auth, etc. checks in the release pipeline steps as most of them aren't necessary at all in that form
  • Added a centralized validation function that validates certain combinations of options that Sentry CLI doesn't do (these validations are also present in the webpack plugin, albeit in a little different form)
  • Added special validation of the release value as it simply has to be present to continue with release injection and the release creation pipeline
  • All validation errors will cause the plugin to throw unless the user specified an error handler. Happy to discuss this point but I think it makes sense to only continue with valid options.

ref #91

@Lms24 Lms24 requested review from lobsterkatie and mydea November 10, 2022 13:31
Comment on lines -102 to -103
if (auto || (repo && commit)) {
await ctx.cli.releases.setCommits(options.release, {
Copy link
Copy Markdown
Member Author

@Lms24 Lms24 Nov 10, 2022

Choose a reason for hiding this comment

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

After seeing that the unit tests fail, I'm realizing that although with this PR we can rely on the global validation, this check here still served a purpose for this unit of code... (hence the unit tests that check for the portions of invalid option here and in addDeploy fail).

Which makes me think... do we prefer central validation (like in this PR currently) or local validation? I could also refactor this and only validate in the respective steps. Open for opinions on this before I adapt tests.

I think the downside of local validation would be that users could end up with a release pipeline that went halfway through before failing (or skipping) e.g. commit or deploy info attachment.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I like the idea of global validation, because then we can also adjust our types as it flows through the cli pipeline. For example, validateOptions could return a type predicate that states what kind of exact fields are defined on the options object.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't have a strong preference either way. IMHO central validation is fine here and maybe a bit easier to follow, but I'm ok either way.

Copy link
Copy Markdown
Member Author

@Lms24 Lms24 Nov 10, 2022

Choose a reason for hiding this comment

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

Then central validation it is, thanks!

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

After discussing this offline, we settled on global validation but with limited TS checks because in this particular situation it's quite tricky and would probably require complex typing.

I made however a small adjustment to the user-facing setCommits type to make it clearer that auto and (repo && commit) are supposed to be mutually exclusive. Nevertheless, I left the checks in place and added a warning in case all three options are provided.

Will mark this PR as ready to review once #104 is merged and I rebased

@Lms24 Lms24 mentioned this pull request Nov 10, 2022
5 tasks
@Lms24 Lms24 marked this pull request as draft November 10, 2022 14:32
@Lms24 Lms24 force-pushed the lms-optional-options branch from 2bfa590 to 35a95db Compare November 10, 2022 16:51
Base automatically changed from lms-optional-options to main November 10, 2022 16:57
@Lms24 Lms24 force-pushed the lms-validate-options branch from 18dfe23 to d377f81 Compare November 11, 2022 08:43
@Lms24 Lms24 marked this pull request as ready for review November 11, 2022 08:58
Copy link
Copy Markdown
Member

@mydea mydea left a comment

Choose a reason for hiding this comment

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

🚀

@Lms24 Lms24 merged commit 46ba891 into main Nov 11, 2022
@Lms24 Lms24 deleted the lms-validate-options branch November 11, 2022 09:40
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.

3 participants