Re-generate OpenAPi spec file inside Make target, include latest generated openapi.yaml file, fix openapi.yaml merge conflicts #3522
Conversation
ran as part of every make run.
Kami
changed the title
st2common/st2common/openapi.yaml
Outdated
| @@ -1,3 +1,5 @@ | |||
| # NOTE: This file is auto-generated. Please edit st2common/st2common/openapi.yaml.j2 | |||
| # and then run make .generate-openapi-spec make target | |||
there are no surprised and we are always working with latest generated file.
Makefile
Outdated
|
|
||
| .PHONY: .lint | ||
| .lint: .flake8 .pylint .bandit .st2client-dependencies-check .st2common-circular-dependencies-check .rst-check | ||
| .lint: .generate-openapi-spec .flake8 .pylint .bandit .st2client-dependencies-check .st2common-circular-dependencies-check .rst-check |
There was a problem hiding this comment.
This should use the already existing .lint-api-spec. This should be a no side-effect operation IMO.
There was a problem hiding this comment.
tbh, I also didn't know where exactly to put this target.
It needs to be somewhere so it's executed on every make run so it always gets generated and committed.
If it's skipped and people don't commit it, things will get out of sync and it will cause issues.
There was a problem hiding this comment.
After thinking about it some more, I can't think of a better place.
We basically do a similar thing in st2docs - we generate files in .docs target. Those are files which always need to be re-generated and commited to the repo, same as in this case.
Makefile
Outdated
| echo "" >> conf/st2.conf.sample | ||
| . $(VIRTUALENV_DIR)/bin/activate; python ./tools/config_gen.py >> conf/st2.conf.sample; | ||
|
|
||
| .PHONY: .generate-openapi-spec |
There was a problem hiding this comment.
This tool st2-validate-api-spec also has a --generate option https://github.com/StackStorm/st2/blob/master/st2common/st2common/cmd/validate_api_spec.py#L44. Under the hood, it does use generate_spect but it's better to use the binary IMO. Also, it was intentional I didn't add this to linter automatically. I didn't have a good place to include the generation. IMO, the lint target should just validate. So I'd do the following changes:
.lint: .lint-api-spec .flake8 .pylint
.lint-api-spec: .generate-api-spec .lint-openapi-spec
Am I making sense? This is slightly better than what you've right now. Only slightly.
There was a problem hiding this comment.
I am also fine if you decide to remove --generate from st2-validate-api-spec and create a new tool st2-generate-api-spec. Having something in tools/ sucks because they won't be shipped in packages. Sorry, I know there are too many things going on but I expected us to argue about this :P
There was a problem hiding this comment.
I just used what's used on Circle CI, but I can update both places to use the new binary to make it consistent.
There was a problem hiding this comment.
And yeah, I will probably move it to a new binary and change functionality so it returns a string and doesn't write anything to a file.
This way it can be used in the same and more flexible manner as before.
a string instead of writing a file. This way it offers more flexibility and we can use it in combination with echo, etc. Also update affected code.
Noticed tests started failing locally after @humblearner API keys pagination change.
It turns out it was @lakshmi-kannan's change. He made a change so we now generate final openapi.yaml file from a Jianja template file.
This pull request fixes conflict with @humblearner's and other PR's which touched
openapi.yamland also adds local Make target to generate final YAML file.Keep in mind that we should always generate final YAML file locally and include generated file in git. This way there is no magic and surprised and you get what you see.
Going forward, we also need to be careful to not manually edit
openapi.yamlfile directly, but edit .j2 file and then run make target to generate finalopenapi.yamlfile.