fix: extract ref from GitOps URL query param (#19)

The backend should extract the ref query param from the GitOps URL and not from the request URL.

Co-authored-by: William Tam <[email protected]>

Author: chetan-rns

pkg/httpapi/api.go

@@ -60,7 +60,7 @@ func (a *APIRouter) GetPipelines(w http.ResponseWriter, r *http.Request) {
 
 	// TODO: replace this with logr or sugar.
 	log.Printf("urlToFetch = %#v\n", urlToFetch)
-	repo, err := parseURL(urlToFetch)
+	repo, parsedRepo, err := parseURL(urlToFetch)
 	if err != nil {
 		log.Printf("ERROR: failed to parse the URL: %s", err)
 		http.Error(w, err.Error(), http.StatusBadRequest)
@@ -85,7 +85,7 @@ func (a *APIRouter) GetPipelines(w http.ResponseWriter, r *http.Request) {
 	// Add a "not found" error that can be returned, otherwise it's a
 	// StatusInternalServerError.
 	log.Println("got an authenticated client")
-	body, err := client.FileContents(r.Context(), repo, "pipelines.yaml", refFromQuery(r.URL.Query()))
+	body, err := client.FileContents(r.Context(), repo, "pipelines.yaml", refFromQuery(parsedRepo.Query()))
 	if err != nil {
 		log.Printf("ERROR: failed to get file contents for repo %#v: %s", repo, err)
 		http.Error(w, err.Error(), http.StatusBadRequest)
@@ -114,7 +114,7 @@ func (a *APIRouter) GetApplication(w http.ResponseWriter, r *http.Request) {
 
 	// TODO: replace this with logr or sugar.
 	log.Printf("urlToFetch = %#v\n", urlToFetch)
-	repo, err := parseURL(urlToFetch)
+	repo, parsedRepo, err := parseURL(urlToFetch)
 	if err != nil {
 		log.Printf("ERROR: failed to parse the URL: %s", err)
 		http.Error(w, err.Error(), http.StatusBadRequest)
@@ -138,7 +138,7 @@ func (a *APIRouter) GetApplication(w http.ResponseWriter, r *http.Request) {
 	//
 	// Add a "not found" error that can be returned, otherwise it's a
 	// StatusInternalServerError.
-	body, err := client.FileContents(r.Context(), repo, "pipelines.yaml", refFromQuery(r.URL.Query()))
+	body, err := client.FileContents(r.Context(), repo, "pipelines.yaml", refFromQuery(parsedRepo.Query()))
 	if err != nil {
 		log.Printf("ERROR: failed to get file contents for repo %#v: %s", repo, err)
 		http.Error(w, err.Error(), http.StatusBadRequest)
@@ -179,12 +179,12 @@ func (a *APIRouter) getAuthenticatedGitClient(fetchURL, token string) (git.SCM,
 	return a.gitClientFactory.Create(fetchURL, token)
 }
 
-func parseURL(s string) (string, error) {
+func parseURL(s string) (string, *url.URL, error) {
 	parsed, err := url.Parse(s)
 	if err != nil {
-		return "", fmt.Errorf("failed to parse %#v: %w", s, err)
+		return "", nil, fmt.Errorf("failed to parse %#v: %w", s, err)
 	}
-	return strings.TrimLeft(strings.TrimSuffix(parsed.Path, ".git"), "/"), nil
+	return strings.TrimLeft(strings.TrimSuffix(parsed.Path, ".git"), "/"), parsed, nil
 }
 
 func secretRefFromQuery(v url.Values) (types.NamespacedName, bool) {

pkg/httpapi/api_test.go

@@ -50,9 +50,9 @@ func TestGetPipelines(t *testing.T) {
 func TestGetPipelinesWithASpecificRef(t *testing.T) {
 	ts, c := makeServer(t)
 	c.addContents("example/gitops", "pipelines.yaml", testRef, "testdata/pipelines.yaml")
-	pipelinesURL := "https://github.com/example/gitops.git"
+	pipelinesURL := fmt.Sprintf("https://github.com/example/gitops.git?ref=%s", testRef)
 
-	req := makeClientRequest(t, "Bearer testing", fmt.Sprintf("%s/pipelines?url=%s&ref=%s", ts.URL, pipelinesURL, testRef))
+	req := makeClientRequest(t, "Bearer testing", fmt.Sprintf("%s/pipelines?url=%s", ts.URL, pipelinesURL))
 	res, err := ts.Client().Do(req)
 	if err != nil {
 		t.Fatal(err)
@@ -225,10 +225,9 @@ func TestGetPipelineApplicationWithRef(t *testing.T) {
 		a.resourceParser = stubResourceParser(testResource)
 	})
 	c.addContents("example/gitops", "pipelines.yaml", testRef, "testdata/pipelines.yaml")
-	pipelinesURL := "https://github.com/example/gitops.git"
+	pipelinesURL := fmt.Sprintf("https://github.com/example/gitops.git?ref=%s", testRef)
 	options := url.Values{
 		"url": []string{pipelinesURL},
-		"ref": []string{testRef},
 	}
 	req := makeClientRequest(t, "Bearer testing",
 		fmt.Sprintf("%s/environments/%s/application/%s?%s", ts.URL, "dev", "taxi", options.Encode()))
@@ -273,11 +272,18 @@ func TestParseURL(t *testing.T) {
 	}
 
 	for _, tt := range urlTests {
-		repo, err := parseURL(tt.u)
+		repo, got, err := parseURL(tt.u)
 		if !test.MatchError(t, tt.wantErr, err) {
 			t.Errorf("got an unexpected error: %v", err)
 			continue
 		}
+		if err == nil {
+			want, err := url.Parse(tt.u)
+			assertNoError(t, err)
+			if got.String() != want.String() {
+				t.Errorf("Parsed URL mismatch: got %v, want %v", got.String(), want.String())
+			}
+		}
 		if repo != tt.wantRepo {
 			t.Errorf("repo got %s, want %s", repo, tt.wantRepo)
 		}