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

feat(1958): Hapi v19 dependencies upgrade. BREAKING CHANGE: v19.x.x #108

Merged
merged 4 commits into from
Aug 26, 2020

Conversation

pritamstyz4ever
Copy link
Contributor

Context

Screwdriver hapi.js dependencies are outdated and not supported anymore.

Objective

This PR upgrades hapi and node fixes all schema related changes to be compatible with latest @Hapi js dependencies.

References

screwdriver-cd/screwdriver#1958

License

I confirm that this contribution is made under the terms of the license found in the root directory of this repository's source tree and that I have the authority necessary to make this contribution on behalf of its copyright owner.

@pritamstyz4ever pritamstyz4ever changed the title Hapi v19 feat: hapi v19 upgrade Aug 4, 2020
package.json Outdated
Comment on lines 56 to 57
"screwdriver-data-schema": "git://github.com/screwdriver-cd/data-schema.git#hapi-v19",
"screwdriver-workflow-parser": "git://github.com/screwdriver-cd/workflow-parser.git#hapi-v19",
Copy link
Member

Choose a reason for hiding this comment

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

reminder to update before merge

@@ -124,7 +124,7 @@ describe('config parser', () => {
() => parser(loadData('bad-step-name.yaml'))
.then((data) => {
assert.match(data.jobs.main[0].commands[0].command,
/"foo bar" only supports the following characters A-Z,a-z,0-9,-,_/);
/foo bar" only supports the following characters A-Z,a-z,0-9,-,_/);
Copy link
Member

Choose a reason for hiding this comment

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

is this weird? why is it missing a leading quotation mark?

Copy link
Contributor Author

@pritamstyz4ever pritamstyz4ever Aug 25, 2020

Choose a reason for hiding this comment

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

The complete message is actually [Validation Error] "jobs.main.steps[0].foo bar" ..., so we are matching the pattern.

Copy link
Member

@jithine jithine Aug 25, 2020

Choose a reason for hiding this comment

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

shouldn't it have matched even with the "?

Copy link
Contributor Author

@pritamstyz4ever pritamstyz4ever Aug 25, 2020

Choose a reason for hiding this comment

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

Actual error message now includes the entire object structure.
ValidationError: "jobs.main.steps[0].foo bar" only supports the following characters A-Z,a-z,0-9,-,_
which is not a correct regex in itself. So made a limited change to message.

Copy link
Member

Choose a reason for hiding this comment

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

missed that

@pritamstyz4ever pritamstyz4ever changed the title feat: hapi v19 upgrade feat(1958): Hapi v19 dependencies upgrade. BREAKING CHANGE: v19.x.x Aug 25, 2020
})
);
return data;
} catch (err) {
Copy link
Member

Choose a reason for hiding this comment

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

why is this catch needed ? can't the entire try..catch be omitted and let the function throw err

@@ -124,7 +124,7 @@ describe('config parser', () => {
() => parser(loadData('bad-step-name.yaml'))
.then((data) => {
assert.match(data.jobs.main[0].commands[0].command,
/"foo bar" only supports the following characters A-Z,a-z,0-9,-,_/);
/foo bar" only supports the following characters A-Z,a-z,0-9,-,_/);
Copy link
Member

@jithine jithine Aug 25, 2020

Choose a reason for hiding this comment

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

shouldn't it have matched even with the "?

@pritamstyz4ever pritamstyz4ever merged commit 266f0ea into master Aug 26, 2020
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