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.

Sign up for GitHub

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

Jump to bottom

fix/nil-runtime-cfg #86

Merged
merged 6 commits into from
Jan 24, 2024
Merged

fix/nil-runtime-cfg #86

merged 6 commits into from
Jan 24, 2024

Conversation

puffitos
Copy link
Collaborator

Motivation

Changes

Added the continue mentioned in the issue.

To properly test the new changes I had to implement the Shutdown function for the loader, to properly shutdown the loader and not just let the Run goroutine end alone.

Tests done

  • New unit tests
  • A manual E2E test where I couldn't load the remote RuntimeConfig from Gitlab was also sucessfull -> no panics.

TODO

  • I've assigned this PR to myself
  • I've labeled this PR correctly

@puffitos
fix: nil check will will not be sent down the channel
d303f90 
Signed-off-by: Bruno Bressi <bruno.bressi@telekom.de>
@puffitos
test: cfg passed to channel/not tested
f069def 
Signed-off-by: Bruno Bressi <bruno.bressi@telekom.de>
@puffitos
chore: added link to issue
b0f3795 
Signed-off-by: Bruno Bressi <bruno.bressi@telekom.de>
@puffitos puffitos self-assigned this Jan 23, 2024
lvlcn-t
lvlcn-t approved these changes Jan 23, 2024
View reviewed changes
Copy link
Collaborator

@lvlcn-t lvlcn-t left a comment

Choose a reason for hiding this comment

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

LGTM, only minor things that can also be done in the future 👍

@puffitos
chore: defer mock cleanup like in other tests
179241f 
Signed-off-by: Bruno Bressi <bruno.bressi@telekom.de>
@puffitos
test: added the client error test
26834a4 
Signed-off-by: Bruno Bressi <bruno.bressi@telekom.de>
@puffitos
chore: reset mock between test cases
e14359f 
Signed-off-by: Bruno Bressi <bruno.bressi@telekom.de>
@lvlcn-t lvlcn-t added the bug Something isn't working label Jan 24, 2024
y-eight
y-eight approved these changes Jan 24, 2024
View reviewed changes
Copy link
Member

@y-eight y-eight left a comment

Choose a reason for hiding this comment

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

LGTM, thx!

@puffitos puffitos merged commit 6b006c3 into main Jan 24, 2024
@lvlcn-t lvlcn-t deleted the fix/nil-runtime-cfg branch January 24, 2024 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@y-eight y-eight y-eight approved these changes

@lvlcn-t lvlcn-t lvlcn-t approved these changes

@niklastreml niklastreml Awaiting requested review from niklastreml niklastreml is a code owner

@eumel8 eumel8 Awaiting requested review from eumel8

Assignees

@puffitos puffitos

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bug: Nil pointer in http loader if endpoint is not reachable Feature: Shutdown for loaders
3 participants
@puffitos @y-eight @lvlcn-t