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

feat: app push strategies #386

Merged
merged 4 commits into from
Feb 29, 2024
Merged

Conversation

Gourab1998
Copy link
Contributor

fix: #377

@Gourab1998 Gourab1998 requested a review from ckganesan February 22, 2024 17:55
@Gourab1998
Copy link
Contributor Author

Hi @sneal @ckganesan

A gentle reminder to get this PR reviewed🙂

Thanks

@sneal
Copy link
Contributor

sneal commented Feb 28, 2024

Overall I like this, we can continue to refine this functionality as needed. I just have one very minor comment around the ToString().

Can you think how we can add tests for this? I don't think we need to add them now before merging this, but it would good to start thinking how we can ensure we don't break this in the future. This is probably getting into the realm of too complex to test without using an integration test against a real running CF deployment or a more fully featured CF API simulator.

@Gourab1998
Copy link
Contributor Author

We generally prefer to use go-vcr in these kind of usecases where

  • Manually mocking all api endpoints is really tough
  • Having an integration test only justifies the complexity
  • We don't have a landscape dedicatedly just to run CI

I feel this might be nice to be added as a future scope here.

@Gourab1998 Gourab1998 requested a review from sneal February 28, 2024 06:47
@sneal
Copy link
Contributor

sneal commented Feb 29, 2024

I like the go-vcr suggestion I'll have to take a look at it.

@sneal sneal merged commit 1aff920 into cloudfoundry:main Feb 29, 2024
@Gourab1998 Gourab1998 deleted the init_strategy_1 branch March 1, 2024 03:22
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.

Support other deployment strategies for CF Push
3 participants