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

Handle gittracks flag #176

Merged
merged 6 commits into from Sep 24, 2019
Merged

Handle gittracks flag #176

merged 6 commits into from Sep 24, 2019

Conversation

DanielMorsing
Copy link
Contributor

Due to thumbs for fingers, I managed to close the last PR by merging it into another branch

This PR adds the gittrack mode flag and adds tests for watches being disabled when handling them is disabled

@DanielMorsing
Copy link
Contributor Author

/retest

@DanielMorsing
Copy link
Contributor Author

see #171 for previous comments and context

Copy link
Contributor

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

Got one nit but other than that everything looks good

Any chance you could squash the commits a little bit? Don't tend to like fixup and WIP in commit messages if possible, commits should be a logical history of the functional changes by the time they hit master


var key = types.NamespacedName{Name: "example", Namespace: "default"}
var expectedRequest = reconcile.Request{NamespacedName: key}

const timeout = time.Second * 5
const consistentTimeout = 1 * time.Second
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit but we normally have this called consistentlyTimeout in our other tests

@DanielMorsing
Copy link
Contributor Author

Fixed up that last suggestion and pruned the history

JoelSpeed
JoelSpeed previously approved these changes Sep 24, 2019
Copy link
Contributor

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

LGTM

@DanielMorsing
Copy link
Contributor Author

/retest

Copy link
Contributor

@mthssdrbrg mthssdrbrg left a comment

Choose a reason for hiding this comment

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

LGTM (I think). There's one comment that I believe is incorrect (or the test is incorrect 😛)

pkg/controller/gittrack/gittrack_controller_test.go Outdated Show resolved Hide resolved
@DanielMorsing
Copy link
Contributor Author

/retest

@DanielMorsing DanielMorsing merged commit 52640ba into master Sep 24, 2019
@pusher-ci pusher-ci deleted the handleGittracks-flag branch September 24, 2019 15:31
@DanielMorsing DanielMorsing restored the handleGittracks-flag branch September 25, 2019 08:46
@DanielMorsing DanielMorsing added this to In progress in Fix Ownership bug via automation Sep 26, 2019
@JoelSpeed JoelSpeed moved this from In progress to Done in Fix Ownership bug Oct 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants