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

PYIC-5872: Open dev internal api gateway again #1982

Merged
merged 2 commits into from
Jun 6, 2024

Conversation

Wynndow
Copy link
Contributor

@Wynndow Wynndow commented Jun 6, 2024

Proposed changes

What changed

Open dev internal api gateway again.

I've tested that this will actually deploy to a non-dev environment.

Why did it change

Revert "Revert "PYIC-5872: Open API gateway""

This reverts commit 1143d1c.

This reversion commit was for everthing in #1966.
Reverting the reversion to bring the changes back in, and then roll forward in the next commit.

PYIC-5872: Use lambda perm over conditional event

Having conditional events on functions wasn't working in non dev
environments. It appears CloudFormation wanted to resolve the nested
reference to the conditionally created API gateway before noticing that
the event itself didn't need creating. This led to errors due to an
unresolveable reference.

Having inspected the CloudFormation stacks created with and without the
event, the only extra resource being created was the permission on the
lambda to allow the testing API gateway to invoke it. All the other API
gateway resources that it might create were already handled by using the
OpenApi spec when creating the gateway itself.

We can create that permission separately and use a conditional for it.

Issue tracking

@Wynndow Wynndow requested review from a team as code owners June 6, 2024 09:19
Having conditional events on functions wasn't working in non dev
environments. It appears CloudFormation wanted to resolve the nested
reference to the conditionally created API gateway before noticing that
the event itself didn't need creating. This led to errors due to an
unresolveable reference.

Having inspected the CloudFormation stacks created with and without the
event, the only extra resource being created was the permission on the
lambda to allow the testing API gateway to invoke it. All the other API
gateway resources that it might create were already handled by using the
OpenApi spec when creating the gateway itself.

We can create that permission separately and use a conditional for it.
@Wynndow Wynndow force-pushed the pyic-5872-open-internal-api-gateway-again branch from cf1ef54 to cc44783 Compare June 6, 2024 09:38
Copy link

sonarqubecloud bot commented Jun 6, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

Copy link
Contributor

@Joe-Edwards-GDS Joe-Edwards-GDS left a comment

Choose a reason for hiding this comment

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

Nice that we don't have a load of extra cfn-lint issues :)

This approach does seem harder to keep up-to-date, but hopefully:

  • In future we'll have a bunch of automated tests using this
  • We don't add new internal APIs very often!

RestApiId: !Ref IPVCorePrivateAPI
Path: /journey/{journeyStep+}
Method: POST
IPVCoreInternalTestingApi:
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this one still be here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep - this approach works when attaching to a step function, rather than a lambda function. Thanks CloudFormation.

Copy link
Contributor

Choose a reason for hiding this comment

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

of course

@Wynndow
Copy link
Contributor Author

Wynndow commented Jun 6, 2024

Nice that we don't have a load of extra cfn-lint issues :)

This approach does seem harder to keep up-to-date, but hopefully:

* In future we'll have a bunch of automated tests using this

* We don't add new internal APIs very often!

Yeah - i've intentionally put the new permissions with the functions rather than all grouped with the new gateway. I'm guessing that the next time we add an internal function we'll look at prior art and it'll help future us remember.

@Wynndow Wynndow merged commit 44bf8dc into main Jun 6, 2024
8 checks passed
@Wynndow Wynndow deleted the pyic-5872-open-internal-api-gateway-again branch June 6, 2024 13:14
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