Skip to content
This repository has been archived by the owner on Oct 11, 2024. It is now read-only.

[FIX] ignore 404 when deleting an API, fixes #469 #541

Merged
merged 20 commits into from
Dec 13, 2022
Merged

[FIX] ignore 404 when deleting an API, fixes #469 #541

merged 20 commits into from
Dec 13, 2022

Conversation

patriziobruno
Copy link

@patriziobruno patriziobruno commented Dec 2, 2022 •
edited
Loading

Ignore not-found error when attempting to delete an API.

Related Issue

#469

Motivation and Context

ApiDefinition CR get stuck when API-definition has been deleted using the dashboard.

Test Coverage For This Change

  • Create an API-definition using tyk-operator
  • Delete the API-definition using the dashboard
  • Delete the API-definition using tyk-operator
  • Verify that tyk-operator removed the api-definition CR

Types of changes

  • 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 change)

Checklist

  • Make sure you are requesting to pull a topic/feature/bugfix branch (right side). If PRing from your fork, don't come from your master!
  • Make sure you are making a pull request against our master branch (left side). Also, it would be best if you started your change off our latest master.
  • Make sure you are updating CHANGELOG.md based on your changes.
  • My change requires a change to the documentation.
    • If you've changed APIs, describe what needs to be updated in the documentation.
  • I have updated the documentation accordingly.
  • If you've changed API models, please update CRDs.
    • make manifests
    • make helm
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • Check your code additions will not fail linting checks:
    • gofmt -s -w .
    • go vet ./...
    • golangci-lint run

@patriziobruno patriziobruno requested a review from a team as a code owner December 2, 2022 11:46
@patriziobruno patriziobruno requested review from komalsukhani and removed request for a team December 2, 2022 11:46
@patriziobruno patriziobruno changed the title ignore 404 when deleting an API, fixes #469 [FIX] ignore 404 when deleting an API, fixes #469 Dec 2, 2022
@komalsukhani
Copy link
Collaborator

Can you add tests for this scenario?

komalsukhani
komalsukhani reviewed Dec 5, 2022
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@caroltyk caroltyk added this to the v0.13.0 milestone Dec 6, 2022
@patriziobruno
Copy link
Author

patriziobruno commented Dec 6, 2022 •
edited
Loading

Can you add tests for this scenario?

UPDATE: never mind, I figured it out

@komalsukhani I could if I knew how to call the test dashboard-api to delete the API before tyk-operator can. Any pointers on how to call dashboard-api from within the BDD context?

buraksekili
buraksekili reviewed Dec 7, 2022
View reviewed changes
Copy link
Collaborator

@buraksekili buraksekili left a comment

Choose a reason for hiding this comment

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

this looks great to me! thank you @patriziobruno !

I would like to explain something about current bdd tests. At the moment, it is not able to use Dashboard API. It only uses Gateway API. That's why adding a step about dashboard API won't help in bdd tests. So, can you please update your tests; so that it only uses Gateway API instead of Dashboard API?
Moreover, We decided to move with Venom tests instead of bdd tests. In the future, our plan is to migrate bdd tests to Venom gradually. Sorry for the confusion.

bdd/godogs_test.go Outdated Show resolved Hide resolved
bdd/godogs_test.go Outdated Show resolved Hide resolved
buraksekili
buraksekili approved these changes Dec 8, 2022
View reviewed changes
Copy link
Collaborator

@buraksekili buraksekili left a comment

Choose a reason for hiding this comment

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

LGTM unless anyone else has any opinions! Once QA finishes testing this PR, we can merge it for the next release. thank you for your great effort @patriziobruno !

@singhpr singhpr merged commit 9b9027d into TykTechnologies:master Dec 13, 2022
@caroltyk
Copy link
Collaborator

Thanks for the contribution @patriziobruno ! The change is now merged and will be included in the next release targeted at early Jan.

buger pushed a commit that referenced this pull request May 22, 2024
@patriziobruno
[FIX] ignore 404 when deleting an API, fixes #469 (#541)
1a85be2 
* ignore 404 when deleting an API, fixes #469

* Update CHANGELOG.md

* typo

* check for 404 error using tykclient.IsNotFound

* move change mention to Unreleased

* add bdd test scenario for deleting ApiDefinition not present in dashboard

* use a separate sample resource to test deletion of deleted apis

* fix linter

* fix createDashURL

* fix ::set-output warnings

* fix typos in feature files

* make bdd tests more reliable

* fix actionlint errors

* use gateway-api for deletion bdd test

* disable bdd strict

* change order of bdd example resources

* split apidefinition from context to fix test

* typos

* fix test

* (re)disable the failing test
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@komalsukhani komalsukhani komalsukhani left review comments

@buraksekili buraksekili buraksekili approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.13.0
Development

Successfully merging this pull request may close these issues.
5 participants
@patriziobruno @komalsukhani @caroltyk @buraksekili @singhpr