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 Concourse registry-image schema #777

Merged
merged 3 commits into from
Jun 2, 2022
Merged

Update Concourse registry-image schema #777

merged 3 commits into from
Jun 2, 2022

Conversation

LukeBalizet
Copy link
Contributor

Update Concourse registry-image resource schema to match concourse/registry-image-resource

Sources:

@kdvolder
Copy link
Member

kdvolder commented May 30, 2022

Thanks a lot for the PR. Indeed it looks like 'registry-image' resource schema is in need of some TLC and has fallen a bit behind on recent (or even not so recent :-) changes.

I took a quick peek at the PR and it looks good on the surface. However besides changes to the schema we also need a few other things:

  • markdown snippets documentation snippets for all the new properties (these show up when hovering a property).
  • new test cases for the added functionality
  • ensure any breaking test cases amongst old cases are fixed. (Test cases break because some added schema elements may show up 'unexpectedly' in existing test cases. So the tests just need updating so they accept this new output as 'expected' instead of failing with an error.

For some examples of existing markdown snippets see here:

For the test cases... usually for every new property added we try to ensure that it is covered by two test at least:

  • one case uses the property in a source code snippet and this validates that the editor understands the property is part of the schema (so doesn't give a 'unknown property' warning'). Usually this test case also tries to validate the schema expresses the correct type information (e.g. if we expect a 'boolean' then we can put something like 'not-a-bool' in the sample and we'd expect it should be flagged as an error with some message like 'expecting a booleam'.
  • one case checks that the new property has a 'meaningful' hover doc (the check usually consists of looking for some text snippet we expect in a 'meaningful' explanation to be a substring of the actual hover text.

For some examples of these kinds of test cases you can look here:

Another possible test case to add would be to check for expected completions are working. This is not mandatory. If the schema validation works, then there is little doubdt that the property is part of the schema and so completions will work (this sort of thing is covered adequately by other test cases).

A tip for finding a good place to work in new properties in the test cases. Choose a property that existed before in a 'similar' location of the schema. Use text search to search it in the source code of the repo. You will find some occurrences inside the ConcourseEditorTest class. This is usually a good place to extend the tests and cover additional / similar properties.

@kdvolder
Copy link
Member

A tip for running the tests:

Copy link
Member

@kdvolder kdvolder left a comment

Choose a reason for hiding this comment

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

Before merging I would like to see some test cases added and the missing markdown snippets for the newly added properties. I've added more detailed explanation about this in the main PR discussion thread.

@kdvolder
Copy link
Member

kdvolder commented Jun 1, 2022

@LukeBalizet This is probably a lot of work or you. If you want to do it, that is great! But if not, I can take it over from you and do the required polishing (test cases, markdown snippets etc).

What would you prefer? Do you want to do the test cases and hover snippets, or shall I take it over?

Either one is fine by me and I don't want to take this from you if you are really eager to contribute these fixes. Just let me know. I may actually have some time tomorrow to look at this.

@LukeBalizet
Copy link
Contributor Author

@kdvolder, yeah it might be better for you to take over 😅 I added the markdown snippets yesterday, but feel free to change them in any way. Thank you for all the work!

@kdvolder
Copy link
Member

kdvolder commented Jun 2, 2022

it might be better for you to take over

Sure, will do! Thanks for the PR and taking it this far. You already went 'the extra mile' to do part of the work. Most people just raise a ticket without any PR at all.

@kdvolder
Copy link
Member

kdvolder commented Jun 2, 2022

@kdvolder kdvolder merged commit a72d422 into spring-projects:main Jun 2, 2022
@kdvolder
Copy link
Member

kdvolder commented Jun 2, 2022

It is merged with a few little problems fixed. Extra test cases, and a few 'enhancements'. E.g. added an emum for the 'AWSRegion' instead of leaving it just a string.

You can try out snapshot

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.

3 participants