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

refactor: remove unnecessary retry logic from data injection #2867

Merged
merged 1 commit into from
Aug 15, 2024

Conversation

phillebaba
Copy link
Member

Description

This change removes the retrying in data injections, as it does not have any effect. The current implementation will only retry at a single condition which is when the pods returned from the wait function is empty.

if len(pods) < 1 {
continue
}

This event wont actually happen because the wait logic will run until the context cancelled if the pod list is empty. This means that returning an empty list is impossible.

Removing the nested retry implementation increases legibility.

Related Issue

Depends on #2856

Checklist before merging

@phillebaba phillebaba requested review from a team as code owners August 12, 2024 09:24
@phillebaba phillebaba changed the title Data retry fix: remove unnecessary retry logic from data injection Aug 12, 2024
Copy link

codecov bot commented Aug 12, 2024

Codecov Report

Attention: Patch coverage is 0% with 56 lines in your changes missing coverage. Please review.

Files Patch % Lines
src/pkg/cluster/data.go 0.00% 56 Missing ⚠️
Files Coverage Δ
src/pkg/cluster/data.go 0.00% <0.00%> (ø)

Signed-off-by: Philip Laine <philip.laine@gmail.com>
@phillebaba phillebaba changed the title fix: remove unnecessary retry logic from data injection refactor: remove unnecessary retry logic from data injection Aug 13, 2024
@phillebaba phillebaba changed the title refactor: remove unnecessary retry logic from data injection refactor: remove unnecessary retry logic from data injection Aug 13, 2024
@schristoff schristoff added the needs-tests PR Label - Tests required to merge label Aug 13, 2024
Copy link
Contributor

@schristoff schristoff left a comment

Choose a reason for hiding this comment

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

tests please

@phillebaba
Copy link
Member Author

The data injection can not be tested in its current state as it is shelling out to both tar and kubectl. What exactly should be tested here maybe other than waitForPodsAndContainers ?

@schristoff
Copy link
Contributor

Yeah, upon re-review it will be too burdensome to try and write functional tests for HandleDataInjection without a major refactor of the functionality itself.

@schristoff schristoff removed the needs-tests PR Label - Tests required to merge label Aug 14, 2024
@schristoff schristoff added this pull request to the merge queue Aug 14, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 14, 2024
@phillebaba phillebaba added this pull request to the merge queue Aug 15, 2024
Merged via the queue into main with commit 8100c69 Aug 15, 2024
26 of 28 checks passed
@phillebaba phillebaba deleted the data-retry branch August 15, 2024 09:35
mjnagel pushed a commit to defenseunicorns/uds-core that referenced this pull request Aug 23, 2024
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Update | Change |
|---|---|---|
| ghcr.io/zarf-dev/packages/init | patch | `v0.38.2` -> `v0.38.3` |
| [zarf-dev/zarf](https://github.com/zarf-dev/zarf) | patch |
`v0.38.2` -> `v0.38.3` |

---

### Release Notes

<details>
<summary>zarf-dev/zarf (zarf-dev/zarf)</summary>

### [`v0.38.3`](https://github.com/zarf-dev/zarf/releases/tag/v0.38.3)

[Compare
Source](https://github.com/zarf-dev/zarf/compare/v0.38.2...v0.38.3)

#### What's Changed

- fix: linter warnings in new Golang CI version by
[@&#8203;phillebaba](https://github.com/phillebaba) in
[zarf-dev/zarf#2883
- chore(deps): bump github.com/go-git/go-git/v5 from 5.11.0 to 5.12.0 by
[@&#8203;dependabot](https://github.com/dependabot) in
[zarf-dev/zarf#2871
- chore(deps): bump github/codeql-action from 3.26.0 to 3.26.1 by
[@&#8203;dependabot](https://github.com/dependabot) in
[zarf-dev/zarf#2881
- test: move oci compose tests that don't need cluster by
[@&#8203;AustinAbro321](https://github.com/AustinAbro321) in
[zarf-dev/zarf#2878
- chore(deps): bump github.com/mikefarah/yq/v4 from 4.44.2 to 4.44.3 by
[@&#8203;dependabot](https://github.com/dependabot) in
[zarf-dev/zarf#2870
- refactor: findImages to return errors immediately by
[@&#8203;phillebaba](https://github.com/phillebaba) in
[zarf-dev/zarf#2851
- test: add workflow to make sure importing Zarf works by
[@&#8203;phillebaba](https://github.com/phillebaba) in
[zarf-dev/zarf#2874
- refactor: remove unnecessary retry logic from data injection by
[@&#8203;phillebaba](https://github.com/phillebaba) in
[zarf-dev/zarf#2867
- docs: explain no wait & helm hooks interaction by
[@&#8203;AustinAbro321](https://github.com/AustinAbro321) in
[zarf-dev/zarf#2895
- refactor: store managed secrets and add tests by
[@&#8203;phillebaba](https://github.com/phillebaba) in
[zarf-dev/zarf#2892
- chore(deps): bump github/codeql-action from 3.26.1 to 3.26.2 by
[@&#8203;dependabot](https://github.com/dependabot) in
[zarf-dev/zarf#2888
- chore(deps): bump actions/setup-go from 5.0.0 to 5.0.2 by
[@&#8203;dependabot](https://github.com/dependabot) in
[zarf-dev/zarf#2901
- fix: update injector by
[@&#8203;AustinAbro321](https://github.com/AustinAbro321) in
[zarf-dev/zarf#2910
- chore(deps): bump github/codeql-action from 3.26.2 to 3.26.4 by
[@&#8203;dependabot](https://github.com/dependabot) in
[zarf-dev/zarf#2916
- fix: update creds not breaking when internal git server not deployed
by [@&#8203;AustinAbro321](https://github.com/AustinAbro321) in
[zarf-dev/zarf#2904
- feat: better error message on helm fail by
[@&#8203;AustinAbro321](https://github.com/AustinAbro321) in
[zarf-dev/zarf#2914
- ci: increase lint timeout by
[@&#8203;AustinAbro321](https://github.com/AustinAbro321) in
[zarf-dev/zarf#2919
- fix: evaulate templates on schema check by
[@&#8203;AustinAbro321](https://github.com/AustinAbro321) in
[zarf-dev/zarf#2911
- chore: update workflow to use new key by
[@&#8203;AustinAbro321](https://github.com/AustinAbro321) in
[zarf-dev/zarf#2920
- ci: permission at job level by
[@&#8203;AustinAbro321](https://github.com/AustinAbro321) in
[zarf-dev/zarf#2922

**Full Changelog**:
zarf-dev/zarf@v0.38.2...v0.38.3

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about these
updates again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR was generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View the
[repository job
log](https://developer.mend.io/github/defenseunicorns/uds-core).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOC4yNi4xIiwidXBkYXRlZEluVmVyIjoiMzguMjYuMSIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOltdfQ==-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants