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.

Sign up for GitHub

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

Jump to bottom

373 deploying the sample data fails #393

Merged
merged 3 commits into from
Feb 6, 2023
Merged

373 deploying the sample data fails #393

merged 3 commits into from
Feb 6, 2023

Conversation

rowan04
Copy link
Contributor

@rowan04 rowan04 commented Nov 8, 2022

Resolves #373

@rowan04 rowan04 requested a review from a team as a code owner November 8, 2022 14:59
tofu-rocketry
tofu-rocketry reviewed Jan 3, 2023
View reviewed changes
Copy link
Member

@tofu-rocketry tofu-rocketry left a comment

Choose a reason for hiding this comment

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

I'd suggest merging the code quality fixes with the first commit, and then splitting that out into a commit around line 39 with a good commit messages explaining why that change, and then a 2nd commit with the new check.

@gregcorbett gregcorbett mentioned this pull request Jan 3, 2023
Merged
@rowan04 rowan04 force-pushed the 373-Deploying_the_sample_data_fails branch from 0f9c798 to 515ef3d Compare January 9, 2023 16:07
@tofu-rocketry
Copy link
Member

We've merged the original commits, and split them into two new ones based on the functionality changed.

( @rowan04 May bad, but I forgot to get you to add a note like this to the PR, which confused @gregcorbett - it's useful when you re-write the history.)

@rowan04
Copy link
Contributor Author

rowan04 commented Jan 12, 2023

We've merged the original commits, and split them into two new ones based on the functionality changed.

( @rowan04 May bad, but I forgot to get you to add a note like this to the PR, which confused @gregcorbett - it's useful when you re-write the history.)

ok!

@rowan04 rowan04 requested a review from gregcorbett January 12, 2023 17:29
gregcorbett
gregcorbett approved these changes Jan 13, 2023
View reviewed changes
Copy link
Member

@gregcorbett gregcorbett left a comment

Choose a reason for hiding this comment

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

LGTM

@gregcorbett gregcorbett force-pushed the 373-Deploying_the_sample_data_fails branch from 2f55dc2 to 7df2485 Compare January 13, 2023 11:23
@gregcorbett gregcorbett added this to the June 2023 milestone Jan 13, 2023
@gregcorbett
Copy link
Member

Rebasing

@rowan04 @gregcorbett
Remove unnecessary LogicException so that sample data deploys correctly
dfe5a59 
- The default statement was set to a LogicException which was always throwing an error as some of the data didn't match the existing case statements
- This was removed and replaced with a break to allow the sample data to deploy correctly
@rowan04 @gregcorbett
Add check that the XML file parsed correctly
f1174c1 
- This is to try and cover the case the removed LogicException was trying to catch
@rowan04 @gregcorbett
Add comment explaining $error object
e10c89c 
- Adds comment explaining that $error will be an object of the libXMLError class
@gregcorbett gregcorbett force-pushed the 373-Deploying_the_sample_data_fails branch from 7df2485 to e10c89c Compare February 6, 2023 10:48
@gregcorbett gregcorbett merged commit c009e73 into GOCDB:dev Feb 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tofu-rocketry tofu-rocketry tofu-rocketry left review comments

@gregcorbett gregcorbett gregcorbett approved these changes

Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
5.11.0
Development

Successfully merging this pull request may close these issues.

Deploying the sample data fails
3 participants
@rowan04 @tofu-rocketry @gregcorbett