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

GitRepo Initial Checkout when DisablePolling is True #2469

Merged
merged 28 commits into from
Jun 12, 2024

Conversation

Tommy12789
Copy link
Contributor

@Tommy12789 Tommy12789 commented May 29, 2024

Refers to #2432

implemented an initial checkout for the gitrepos applied without polling and commit update while force updating

./integrationtests/gitjob/controller/controller_test.go

@Tommy12789 Tommy12789 changed the title first implementation GitRepo Initial Checkout when DisablePolling is True May 29, 2024
@Tommy12789 Tommy12789 marked this pull request as ready for review June 3, 2024 10:21
@Tommy12789 Tommy12789 requested a review from a team as a code owner June 3, 2024 10:21
Copy link
Contributor

@0xavi0 0xavi0 left a comment

Choose a reason for hiding this comment

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

As discussed offline, there is a bug when doing a "Force Update" because the contents are re-deployed but the commit is not updated.

That works because the completed job remains in the cluster and the

	} else if gitRepo.Status.Commit != "" {
		if err = r.deleteJobIfNeeded(ctx, &gitRepo, &job); err != nil {
			return ctrl.Result{}, fmt.Errorf("error deleting git job: %v", err)
		}
	}

code is executed (which recreates the job and fetches the latest from the repository).

But this part:

                        fetcher := git.NewFetcher()
			commit, err := fetcher.LatestCommit(ctx, &gitRepo, r.Client)
			if err != nil {
				return ctrl.Result{}, fmt.Errorf("error fetching commit: %v", err)
			}
			gitRepo.Status.Commit = commit
			logger.V(1).Info("Updating GitRepo status", "commit", gitRepo.Status.Commit)
			if err := r.Status().Update(ctx, &gitRepo); err != nil {
				return ctrl.Result{}, fmt.Errorf("error updating git repo status: %v", err)
			}
			return ctrl.Result{}, nil

is no longer executed and the commit is not updated in the gitrepo.

It is not executed because it deletes the job and then it executes this branch:

     if errors.IsNotFound(err) {
		if gitRepo.Status.Commit != "" {
			if err := r.createJob(ctx, &gitRepo); err != nil {
				return ctrl.Result{}, fmt.Errorf("error creating git job: %v", err)
			}
		}

as gitRepo.Status.Commit is no longer "" like it was for the first time.

Copy link
Contributor

@0xavi0 0xavi0 left a comment

Choose a reason for hiding this comment

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

I guess it would be cool to also test the case that had the bug before, but I guess that requires an e2e test because you need to create a new commit and these tests are relying on the fleet repo in github.
Up to you if you want to experiment and create that.

Aside from the code simplification and the lines that belong to other PRs... LGTM

0xavi0
0xavi0 previously approved these changes Jun 10, 2024
Copy link
Contributor

@0xavi0 0xavi0 left a comment

Choose a reason for hiding this comment

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

LGTM
Thanks for adding the extra e2e test

return out
}).ShouldNot(ContainSubstring(gitrepoName))

out, err := k.Namespace("cattle-fleet-system").Logs(
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: This could be flagged as a failure if any previous test failed.

@0xavi0 0xavi0 self-requested a review June 12, 2024 10:40
@manno manno merged commit bb2a70a into rancher:main Jun 12, 2024
8 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.

3 participants