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

Update vets_json_schema #19442

Closed
wants to merge 7 commits into from
Closed

Conversation

opticbob
Copy link
Contributor

Summary

  • This work is behind a feature toggle (flipper): NO
  • (Summarize the changes that have been made to the platform) I've updated the vets_json_schema gem to the latest version.
  • (If bug, how to reproduce)
  • (What is the solution, why is this the solution?)
  • (Which team do you work for, does your team own the maintenance of this component?) FAR/ARM, we don't own the maintenance.
  • (If introducing a flipper, what is the success criteria being targeted?)

Related issue(s)

Testing done

  • New code is covered by unit tests
  • Describe what the old behavior was prior to the change
  • Describe the steps required to verify your changes are working as expected. Exclusively stating 'Specs run' is NOT acceptable as appropriate testing
  • If this work is behind a flipper:
    • Tests need to be written for both the flipper on and flipper off scenarios. Docs.
    • What is the testing plan for rolling out the feature?

Screenshots

Note: Optional

What areas of the site does it impact?

(Describe what parts of the site are impacted andifcode touched other areas) This just touches the vets_json_schema gem.

Acceptance criteria

  • I fixed|updated|added unit tests and integration tests for each feature (if applicable).
  • No error nor warning in the console.
  • Events are being sent to the appropriate logging solution
  • Documentation has been updated (link to documentation)
  • No sensitive information (i.e. PII/credentials/internal URLs/etc.) is captured in logging, hardcoded, or specs
  • Feature/bug has a monitor built into Datadog (if applicable)
  • If app impacted requires authentication, did you login to a local build and verify all authenticated routes work as expected
  • I added a screenshot of the developed feature

Requested Feedback

(OPTIONAL)What should the reviewers know in addition to the above. Is there anything specific you wish the reviewer to assist with. Do you have any concerns with this PR, why?

@va-vfs-bot va-vfs-bot temporarily deployed to 93506-update-the-vets-json-schema/main/main November 13, 2024 18:00 Inactive
@va-vfs-bot va-vfs-bot temporarily deployed to 93506-update-the-vets-json-schema/main/main November 13, 2024 19:14 Inactive
@va-vfs-bot va-vfs-bot temporarily deployed to 93506-update-the-vets-json-schema/main/main November 14, 2024 14:28 Inactive
@va-vfs-bot va-vfs-bot temporarily deployed to 93506-update-the-vets-json-schema/main/main November 15, 2024 19:09 Inactive
@va-vfs-bot va-vfs-bot temporarily deployed to 93506-update-the-vets-json-schema/main/main November 18, 2024 18:34 Inactive
Copy link
Contributor

@rmtolmach rmtolmach left a comment

Choose a reason for hiding this comment

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

Copy link

Backend-review-group approval confirmed.

@rmtolmach
Copy link
Contributor

oh, your specs had timed out. I'm re-running them for you.

@rmtolmach
Copy link
Contributor

oh, now you're hitting a failing test in master. I'm working with that test owner to get it resolved and then we can update your branch.

@opticbob
Copy link
Contributor Author

@rmtolmach That sounds great thanks! I was just looking at this and it looks like there were a couple of updates and version bumps to the vets-json-schema repo besides mine that never updated the vets_json_schema gem. One update before mine and one after. Do you think the failing tests are related?

@va-vfs-bot va-vfs-bot temporarily deployed to 93506-update-the-vets-json-schema/main/main November 18, 2024 20:28 Inactive
@rmtolmach
Copy link
Contributor

The test is fixed in master, but you're getting failures on your branch (they're not the same ones that were in master) 😕 See here:

Can you investigate those?

@opticbob
Copy link
Contributor Author

Yep, I'll check them out. Thanks @rmtolmach !

@opticbob
Copy link
Contributor Author

Hey @rmtolmach, I need some clarification on how to move forward with this. I'm going to refer to vets-json-schema's package.json history for my timeline.

In department-of-veterans-affairs/vets-json-schema@199861a two weeks ago some regexes were changed around the processing of form 40-10007. The package.json was bumped from 24.5.2 to 24.5.3 in the vets-json-schema repo. The vets_json_schema gem was never updated here in vets-api to reflect that update.

My vets-json-schema PR bumped the version to 24.5.4. It has since been bumped one more time to 24.5.5.

As far as I can tell those earlier vets-json-api changes are causing the test errors we're seeing in this PR now. There is a similar situation going on in the matching PR on vets-website.

How should I move forward with this? Do I need to modify another team's tests here in vets-api so they pass? Should I revert their changes to vets-json-schema so they pass without changes in this repo? Is there another way to make sure the changes I made to vets-json-schema can be used here in the vets_json_schema gem without the test breaking changes coming along for the ride?

@rmtolmach
Copy link
Contributor

I will pass this on to @RachalCassity. She's on support tomorrow and can help.

@opticbob
Copy link
Contributor Author

It looks like there is a PR that would go a long way toward fixing these tests: #19294. I'm not sure why it hasn't been merged.

@opticbob
Copy link
Contributor Author

Hello @RachalCassity ! Do you have a suggestion how I can move forward with this PR as written up above?

@holdenhinkle
Copy link
Contributor

Hi @RachalCassity ! I'm only following this from a high-level, but it looks like this PR needs to be merged first? #19294

It got approvals 2 weeks ago but the author hasn't merged it.

cc @opticbob

@opticbob
Copy link
Contributor Author

This PR was superseded by #19294 which has now merged. The changes I made to vets-json-schema are now available in vets-api.

This PR can be closed without merging.

@opticbob opticbob closed this Nov 22, 2024
@opticbob opticbob deleted the 93506-update-the-vets-json-schema branch November 22, 2024 19:04
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.

5 participants