-
Notifications
You must be signed in to change notification settings - Fork 15
Conversation
20c426f
to
39153af
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add a test case for this before we merge it IMO.
Could you also add a quick code block showing the output of what -o yaml
would look like for the status
field with this change?
Finally, I'd like to see ignores from the handleObject
function printed in this output too, any chance you can work that into this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look reasonable, needs to have some tests though! :)
ed14b21
to
4b0ea93
Compare
@@ -238,7 +238,7 @@ func (r *ReconcileGitTrack) getFiles(gt *farosv1alpha1.GitTrack) (map[string]*gi | |||
return nil, fmt.Errorf("no files for subpath '%s'", gt.Spec.SubPath) | |||
} | |||
|
|||
r.log.V(1).Info("Loaded files from repository", "file count", string(len(files))) | |||
r.log.V(1).Info("Loaded files from repository", "file count", len(files)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated to this PR, but the logging message confuses me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Have we tested this live somewhere? Otherwise that'd be a good exercise to do.
@mthssdrbrg I had tested only before @JoelSpeed's changes |
/build docker |
LGTM @mthssdrbrg @JoelSpeed WDYT?
|
Co-Authored-By: JoelSpeed <Joel.speed@hotmail.co.uk>
60a92c4
to
cb60074
Compare
/retest |
1 similar comment
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (though I authored half of it so @mthssdrbrg or @gargath can you also review)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Displays in status a list of files considered invalid due to parsing errors.
Appears as:
TODO: