Fix a bunch of issues.

api/v1alpha1/config.go

@@ -2,8 +2,9 @@ package v1alpha1
 
 import "strings"
 
-// Config stores the hydros configuration.
-type Config struct {
+// HydrosConfig is hydros GitHub App configuration. This is the configuration that should be checked into
+// a repository to configure hydros on that repository
+type HydrosConfig struct {
 	APIVersion string     `yaml:"apiVersion"`
 	Kind       string     `yaml:"kind"`
 	Metadata   Metadata   `yaml:"metadata"`
@@ -35,7 +36,7 @@ type InPlaceConfig struct {
 
 // IsValid returns true if the config is valid.
 // For invalid config the string will be a message of validation errors
-func IsValid(c *Config) (string, bool) {
+func IsValid(c *HydrosConfig) (string, bool) {
 	errors := make([]string, 0, 10)
 	baseBranches := make(map[string]bool)
 	prBranches := make(map[string]bool)
@@ -53,7 +54,7 @@ func IsValid(c *Config) (string, bool) {
 	}
 
 	if len(errors) > 0 {
-		return "Config is invalid. " + strings.Join(errors, ". "), false
+		return "HydrosConfig is invalid. " + strings.Join(errors, ". "), false
 	}
 	return "", true
 }

pkg/app/config.go

@@ -39,7 +39,11 @@ type App struct {
 	PrivateKey    string `yaml:"private_key" json:"privateKey"`
 }
 
-// BuildConfig is a helper function to build the configuration
+// BuildConfig is a helper function to build the GitHubApp configuration.
+// Palantir libraries use *githubapp.Config for basic configuration. We don't necessarily want to directly use
+// that configuration because not all of those options make sense. For example, we allow URIs to be used
+// for the secrets. So this is a helper function to convert our values into githubapp.Config which
+// can be passed to those libraries.
 func BuildConfig(appId int64, webhookSecret string, privateKeySecret string) (*githubapp.Config, error) {
 	hmacSecret, err := readSecret(webhookSecret)
 	if err != nil {

pkg/app/fetcher.go

@@ -12,7 +12,7 @@ import (
 
 // FetchedConfig represents the hydros configuration fetched from GitHub and used to configure hydros
 type FetchedConfig struct {
-	Config     *v1alpha1.Config
+	Config     *v1alpha1.HydrosConfig
 	LoadError  error
 	ParseError error
 
@@ -39,7 +39,7 @@ func (cf *ConfigFetcher) ConfigForRepositoryBranch(ctx context.Context, client *
 		return fc
 	}
 
-	var pc v1alpha1.Config
+	var pc v1alpha1.HydrosConfig
 	if err := yaml.UnmarshalStrict(c.Content, &pc); err != nil {
 		fc.ParseError = err
 	} else {

pkg/app/handler.go

@@ -31,19 +31,22 @@ const (
 	SharedRepository = ".github"
 )
 
+// HydrosHandler is a handler for certain GitHub events. It currently handles PushEvents by sending them to
+// Renderer which knows how to do in place modification using KRMs.
+// TODO(jeremy): Also handle syncer.
 type HydrosHandler struct {
 	githubapp.ClientCreator
 	Manager *gitops.Manager
 	// TODO(jeremy): ClientCreator and TransportManager are somewhat redundant
 	transports *hGithub.TransportManager
 
 	workDir string
-
 	fetcher *ConfigFetcher
 }
 
 // NewHandler starts a new HydrosHandler for GitHub.
-// config: The GitHub configuration
+// cc: ClientCreator for creating GitHub clients.
+// transports: Manage GitHub transports
 // workDir: The directory to use for storing temporary files and checking out repositories
 // numWorkers: The number of workers to use for processing events
 func NewHandler(cc githubapp.ClientCreator, transports *hGithub.TransportManager, workDir string, numWorkers int) (*HydrosHandler, error) {
@@ -90,11 +93,14 @@ func (h *HydrosHandler) Handles() []string {
 }
 
 func (h *HydrosHandler) Handle(ctx context.Context, eventType, deliveryID string, payload []byte) error {
-	// TODO(jeremy): How do we distinguish between pushing a PR and merging a PR.
 	log := zapr.NewLogger(zap.L()).WithValues("eventType", eventType, "deliverID", deliveryID)
 	log.V(util.Debug).Info("Got github webhook")
 	r := bytes.NewBuffer(payload)
 	d := json.NewDecoder(r)
+
+	// Handle push events.
+	// GitHub push events are sent whenever a branch is pushed. I believe a single push could contain multiple commits.
+	// When a PR is merged, that will trigger a push with the commits pushed to the target branch.
 	event := &github.PushEvent{}
 	if err := d.Decode(event); err != nil {
 		log.Error(err, "Failed to decode a PushEvent")
@@ -110,7 +116,6 @@ func (h *HydrosHandler) Handle(ctx context.Context, eventType, deliveryID string
 		return err
 	}
 
-	// TODO(jeremy): We can use the checks client to create checks.
 	installationID := githubapp.GetInstallationIDFromEvent(event)
 	client, err := h.NewInstallationClient(installationID)
 	if err != nil {
@@ -192,21 +197,11 @@ func (h *HydrosHandler) Handle(ctx context.Context, eventType, deliveryID string
 	rName := gitops.RendererName(repoName.RepoOwner(), repoName.RepoName())
 
 	if !h.Manager.HasReconciler(rName) {
-
-		if ghrepo.FullName(repoName) != "jlewi/hydros-hydrated" {
-			return errors.Errorf("Currently only the repository jlewi/hydros-hydrated. Code needs to be updated to support other repositories")
-		}
 		log.Info("Creating reconciler", "name", rName)
-
-		// TODO(jeremy): This information should come from the configuration checked into the repository.
-		// e.g.from the file .github/hydros.yaml
-
-		org := "jlewi"
-		repo := "hydros-hydrated"
 		// Make sure workdir is unique for each reconciler.
 		workDir := filepath.Join(h.workDir, rName)
 
-		r, err := gitops.NewRenderer(org, repo, workDir, h.transports, client)
+		r, err := gitops.NewRenderer(repoName.RepoOwner(), repoName.RepoName(), workDir, h.transports, client)
 		if err != nil {
 			return err
 		}
@@ -224,7 +219,7 @@ func (h *HydrosHandler) Handle(ctx context.Context, eventType, deliveryID string
 		// https://docs.github.com/en/webhooks-and-events/webhooks/webhook-events-and-payloads#push
 		// "After" is the commit after the push.
 		Commit: event.GetAfter(),
-		// Config could potentially be different for different commits
+		// HydrosConfig could potentially be different for different commits
 		// So we pass it along with the event
 		BranchConfig: inPlaceConfig,
 	})

pkg/app/handler_test.go

@@ -91,7 +91,6 @@ func Test_HookManual(t *testing.T) {
 		t.Fatalf("Failed to handle the push")
 	}
 
-	//time.Sleep(10 * time.Minute)
 	// Wait for it to finish processing.
 	handler.Manager.Shutdown()
 }
@@ -176,9 +175,6 @@ func latestCommit(transports *hGithub.TransportManager, repo ghrepo.Interface, w
 		return "", err
 	}
 
-	// The short tag will be used to tag the artifacts
-	//b.commitTag = ref.Hash().String()[0:7]
-
 	log.Info("Current commit", "commit", ref.Hash().String())
 	return ref.Hash().String(), nil
 }

pkg/gitops/renderer.go

@@ -26,7 +26,7 @@ const (
 	RendererCheckName = "hydros-ai"
 )
 
-// Renderer handles in place modification of YAML files.
+// Renderer is a reconciler that handles in place modification of YAML files.
 // It is intended to run a bunch of KRM functions in place and then check the modifications back into the repository.
 //
 // There is currently one renderer per repository. A single renderer can handle multiple branches but not
@@ -38,28 +38,16 @@ const (
 // when hydrating into a different repository (e.g. via Syncer) but not when changes are to be checked into the
 // source repository.
 type Renderer struct {
-	org  string
-	repo string
-	// ForkRepo is the repo into which the changes will be pushed and the PR created from
-	//ForkRepo *v1alpha1.GitHubRepo `yaml:"forkRepo,omitempty"`
-	//
-	//// DestRepo is the repo into which a PR will be created to merge hydrated
-	//// manifests from the ForkRepo
-	//DestRepo *v1alpha1.GitHubRepo `yaml:"destRepo,omitempty"`
-
-	workDir string
-
-	// repoHelper helps with creating PRs
-	// repoHelper *github.RepoHelper
+	org        string
+	repo       string
+	workDir    string
 	transports *github.TransportManager
 
 	client *ghAPI.Client
 }
 
 func NewRenderer(org string, name string, workDir string, transports *github.TransportManager, client *ghAPI.Client) (*Renderer, error) {
 	r := &Renderer{
-		//ForkRepo:   forkRepo,
-		//DestRepo:   destRepo,
 		org:        org,
 		repo:       name,
 		workDir:    workDir,
@@ -99,6 +87,9 @@ func (r *Renderer) Run(anyEvent any) error {
 		return fmt.Errorf("Event is not a RenderEvent")
 	}
 
+	// TODO(https://github.com/jlewi/hydros/issues/32): The semantics should probably be that we skip running if
+	// we aren't the latest commit. So if the commit is specified we should verify its the latest commit and if not
+	// skip it.
 	if event.Commit == "" {
 		// When no commit is specified we should run on the latest commit. This means
 		// 1. We need to fetch the latest commit
@@ -209,17 +200,17 @@ func (r *Renderer) Run(anyEvent any) error {
 			}
 		}
 
-		if event.Commit != "" {
-			// TODO(jeremy): We need to update PrepareBranch to properly create the branch from the commit.
-			err := errors.Errorf("Commit isn't properly supported yet. The branch is prepared off HEAD and not the commit")
-			log.Error(err, "Commit isn't properly supported yet.", "commit", event.Commit)
-		}
-
 		// Checkout the repository.
 		if err := repoHelper.PrepareBranch(true); err != nil {
 			return err
 		}
 
+		if event.Commit != "" {
+			// TODO(https://github.com/jlewi/hydros/issues/32): We should check whether commit is the latest
+			// commit on the branch and if it isn't we should the run.
+			log.Info("Warning. Commit is set in RenderEvent but code will use code from latest commit regardless of whether it matches the commit. See https://github.com/jlewi/hydros/issues/32.", "commit", event.Commit)
+		}
+
 		syncNeeded, err := r.syncNeeded()
 		if err != nil {
 			return err

pkg/gitops/renderer_test.go

@@ -49,28 +49,19 @@ func Test_RendererManualE2E(t *testing.T) {
 		}
 
 		r := Renderer{
-			//SourceRepo: &v1alpha1.GitHubRepo{
-			//	Org:    "jlewi",
-			//	Repo:   "hydros",
-			//	Branch: "jlewi/ai",
-			//},
-			ForkRepo: &v1alpha1.GitHubRepo{
-				Org:    "jlewi",
-				Repo:   "hydros",
-				Branch: "jlewi/ai-test",
-			},
-			DestRepo: &v1alpha1.GitHubRepo{
-				Org:    "jlewi",
-				Repo:   "hydros",
-				Branch: "jlewi/ai",
-			},
 			workDir:    "/tmp/test_renderer",
-			sourcePath: "tests/manifests",
-			repoHelper: nil,
 			transports: manager,
 		}
 
-		return r.Run()
+		event := RenderEvent{
+			Commit: "",
+			BranchConfig: &v1alpha1.InPlaceConfig{
+				BaseBranch: "main",
+				PRBranch:   "hydros/manual-test",
+				AutoMerge:  false,
+			},
+		}
+		return r.Run(event)
 	}()
 	if err != nil {
 		t.Fatalf("Error running renderer; %v", err)