Skip to content

Improve spec update script#6

Merged
giograno merged 2 commits intomainfrom
enhance-script
Oct 30, 2024
Merged

Improve spec update script#6
giograno merged 2 commits intomainfrom
enhance-script

Conversation

@giograno
Copy link
Member

@giograno giograno commented Oct 28, 2024

This PR improves the python script we use to generate an update version of the spec.

Initially, the script was:

  • using the get_localstack_openapi_spec function from LocalStack core to retrieve the entire spec;
  • setting the version to latest;
  • saving the spec in the destination path (openapi/emulators/localstack-spec-latest.yml).

The script now take as --latest flag to replicate the behavior described above.
If the flag is not provided, the version number is exactraced from LocalStack itself. This give us some flexibility to use the same script in the release pipeline.

Context: the step to publish a tagged spec at release time uses the improved version of the script in this PR.

@giograno giograno requested a review from alexrashed October 28, 2024 13:03
@giograno giograno marked this pull request as ready for review October 28, 2024 13:09
Copy link
Member

@alexrashed alexrashed left a comment

Choose a reason for hiding this comment

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

LGTM! I added a few questions and nitpicks, but nothing blocking ;)

in a file at a given path.
"""
version = "latest" if latest else ls_version
openapi_path = Path(os.path.dirname(__file__)) / ".." / "openapi" / "emulators"/ f"localstack-spec-{version}.yml"
Copy link
Member

Choose a reason for hiding this comment

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

Q: What is the motivation behind adding the version to the filename? I feel like the version field in the spec together with tags and releases should be enough, and with different file names diffing just gets more complicated... I would love to be able to see the differences between two versions of the specs just by looking at the changes in the commits. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

My initial idea was to keep here all the tagged specs plus a latest. But since the specs are actually attached to the release, I do agree that this is redundant.

Comment on lines +11 to +12
@click.command()
@click.option('--latest', is_flag=True, default=False, help='If enabled, sets the version of the spec to latest.')
Copy link
Member

Choose a reason for hiding this comment

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

nit: I'm not sure if it's necessary to add a dependency on click just to add an argument, but that's just a very subjective opinion and click is pretty cool :P

Copy link
Member Author

Choose a reason for hiding this comment

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

As you correctly mentioned above, click comes transitively with LocalStack so I don't see harm using it :)

@giograno giograno merged commit f3aeb1e into main Oct 30, 2024
@giograno giograno deleted the enhance-script branch October 30, 2024 09:44
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.

2 participants