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

fix: adds passage-version header to all api calls #69

Merged
merged 4 commits into from
Oct 17, 2024

Conversation

ctran88
Copy link
Contributor

@ctran88 ctran88 commented Oct 16, 2024

Description

Adds a missing header to requests for tracking the package version.

It looks like OpenAPITools/openapi-generator doesn't expose setting custom headers in the configuration OpenAPITools/openapi-generator#11431, so added a code mod function in the generator to set the additional header code.

Not bumping the version since we have a few more things coming down the pipe we can bundle.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation
  • Infrastructure

Testing

Checklist before approving (including SDK updates)

  • I have performed a review of the code.
  • Docs have been updated. Docs in the docs/Passage folder should be edited with any changes to the api. We manually edit these docs to account for the wrapping layer of the generated code.
  • The version has been updated in the composer.json file.
  • The CHANGELOG.md has been updated.

Checklist after merging

  • Github repo is tagged.

Sorry, something went wrong.

@ctran88 ctran88 marked this pull request as ready for review October 16, 2024 17:28
@ctran88 ctran88 requested a review from a team as a code owner October 16, 2024 17:28
@ctran88 ctran88 force-pushed the PSG-4467-add-passage-version-header branch from 7b06041 to 7038d71 Compare October 16, 2024 17:34
@ctran88 ctran88 force-pushed the PSG-4467-add-passage-version-header branch from 7038d71 to 6ff10d2 Compare October 16, 2024 17:35
Copy link
Collaborator

@vanessa-passage vanessa-passage left a comment

Choose a reason for hiding this comment

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

Sorry, I take back the approval. Can you also add the changes to the CHANGELOG?

I know we are waiting to release, but we should still make it easy on ourselves and make a note of the unreleased changes so we don't have to look back to see what has changed.

@ctran88
Copy link
Contributor Author

ctran88 commented Oct 17, 2024

Sorry, I take back the approval. Can you also add the changes to the CHANGELOG?

I know we are waiting to release, but we should still make it easy on ourselves and make a note of the unreleased changes so we don't have to look back to see what has changed.

Would writing unreleased changes to our changelog be preferable to using the conventional commit messages to aggregate the changes when were ready to release?

I wasn't sure if writing unreleased changes to the log could be confusing to anyone reading it

@vanessa-passage
Copy link
Collaborator

Sorry, I take back the approval. Can you also add the changes to the CHANGELOG?
I know we are waiting to release, but we should still make it easy on ourselves and make a note of the unreleased changes so we don't have to look back to see what has changed.

Would writing unreleased changes to our changelog be preferable to using the conventional commit messages to aggregate the changes when were ready to release?

I wasn't sure if writing unreleased changes to the log could be confusing to anyone reading it

I typically lean more towards an Unreleased section. Us not having a reliable, agreed upon commit message standard, also supports this.

Having an Unreleased section is a fairly common practice. You can see an example here. Unreleased sections can help devs know what is coming and make updates to CHANGELOGS easier when we aren't using something like beachball.

@ctran88
Copy link
Contributor Author

ctran88 commented Oct 17, 2024

I typically lean more towards an Unreleased section. Us not having a reliable, agreed upon commit message standard, also supports this.

Having an Unreleased section is a fairly common practice. You can see an example here. Unreleased sections can help devs know what is coming and make updates to CHANGELOGS easier when we aren't using something like beachball.

added 74fff14

Copy link
Collaborator

Choose a reason for hiding this comment

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

Deleted by accident?

Copy link
Contributor Author

@ctran88 ctran88 Oct 17, 2024

Choose a reason for hiding this comment

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

no, .env should never be committed to a repo. it also opens it up to human error of updating the .env to run tests and accidentally committing the secrets since it's already indexed.

i considered a .env.example but it's really only used for testing, which isn't in a friendly state for an open-source contributor to run anyways. so i adjusted the test setup to look for the file just in case, but otherwise just grab from the environment.

we do plan on updating the tests (and moving them around a bit) to make it friendlier to run by anyone.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that being said, i could still add a .env.example, though we plan on removing the need for it within this initiative

Copy link
Collaborator

Choose a reason for hiding this comment

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

Up to you. If we have a plan for removal we might be fine. I would just be concerned if Tamara or anyone else worked on the php repo and didn't have an example.

@ctran88 ctran88 merged commit 0e98725 into main Oct 17, 2024
1 check passed
@ctran88 ctran88 deleted the PSG-4467-add-passage-version-header branch November 21, 2024 19:06
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.

None yet

2 participants