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

create kind network #67

Merged
merged 5 commits into from
Oct 27, 2023
Merged

Conversation

nabuskey
Copy link
Collaborator

The test assume we have a docker network named "kind". Since the test only cares about registry image, we'll just create the network prior to reconciling.

Note that we will not need this functionality going forward. If we did, I think the way @cmoulliard did with test containers is the way to go. I want to fix tests so we don't run into test failures for unrelated PRs.

ubuntu@ip-10-192-11-119:~/idpbuilder$ go test github.com/cnoe-io/idpbuilder/pkg/kind -run '^\QTestReconcileRegistry\E$'
ok  	github.com/cnoe-io/idpbuilder/pkg/kind	4.495s
ubuntu@ip-10-192-11-119:~/idpbuilder$ docker images
REPOSITORY   TAG       IMAGE ID   CREATED   SIZE
ubuntu@ip-10-192-11-119:~/idpbuilder$ docker network ls
NETWORK ID     NAME      DRIVER    SCOPE
906d8469762b   bridge    bridge    local
90ea394ecb86   host      host      local
44350ca9e837   none      null      local

fixes: #28

@nabuskey nabuskey force-pushed the fix/cluster-registry branch from 322364d to 887542e Compare October 26, 2023 21:59
Signed-off-by: Manabu Mccloskey <[email protected]>
Signed-off-by: Manabu Mccloskey <[email protected]>
@nabuskey nabuskey force-pushed the fix/cluster-registry branch from 887542e to a79b6ec Compare October 26, 2023 22:28
@nimakaviani nimakaviani added this to the v0.1 release milestone Oct 26, 2023
Signed-off-by: Manabu Mccloskey <[email protected]>
Signed-off-by: Manabu Mccloskey <[email protected]>
Signed-off-by: Manabu Mccloskey <[email protected]>
@nabuskey
Copy link
Collaborator Author

nabuskey commented Oct 26, 2023

Well, it passes the tests now. I will say that this is very fragile and I can't reproduce the previous test failures on my Linux system.

Good news is that we don't need these tests going forward.

@nabuskey nabuskey mentioned this pull request Oct 27, 2023
@nimakaviani nimakaviani self-requested a review October 27, 2023 19:25
Copy link
Contributor

@nimakaviani nimakaviani left a comment

Choose a reason for hiding this comment

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

LGTM

yay, first green build. thanks!

Comment on lines +67 to +76
for {
if time.Now().After(endTime) {
t.Fatalf("Timed out waiting for port %d to be available", kind.ExposedRegistryPort)
}
conn, cErr := net.DialTimeout("tcp", net.JoinHostPort("0.0.0.0", strconv.Itoa(int(kind.ExposedRegistryPort))), time.Second*3)
if cErr != nil {
break
}
conn.Close()
time.Sleep(waitInterval)
Copy link
Contributor

Choose a reason for hiding this comment

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

cosmetics here but pushing the check to a go func would be nicer

Suggested change
for {
if time.Now().After(endTime) {
t.Fatalf("Timed out waiting for port %d to be available", kind.ExposedRegistryPort)
}
conn, cErr := net.DialTimeout("tcp", net.JoinHostPort("0.0.0.0", strconv.Itoa(int(kind.ExposedRegistryPort))), time.Second*3)
if cErr != nil {
break
}
conn.Close()
time.Sleep(waitInterval)
ready := make(chan bool)
endTime := time.After(waitTimeout)
go func() {
for {
....
if cErr == nil {
close(ready)
}
}
}()
select {
case <-timeout:
err := errors.New("Timeout")
log.Error(err, "Didn't reconcile Gitea on time.")
return ctrl.Result{}, err
case <-ready:
log.Info("Gitea is ready!")
resource.Status.GiteaAvailable = true
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm using the same format that was used here:

endTime := time.Now().Add(timeout)

If we are using timeout, we should use the same logic everywhere or make it reusable. not worth the time effort imo.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah not a big deal, lets get it over the line.

Comment on lines +39 to +43
endTime := time.Now().Add(waitTimeout)

for {
if time.Now().After(endTime) {
t.Fatalf("Timed out waiting for registry. recent error: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

@nimakaviani nimakaviani merged commit 6b32abf into cnoe-io:main Oct 27, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix tests
2 participants