Skip to content
This repository has been archived by the owner on Dec 10, 2024. It is now read-only.

Add support for hidden field to project variables #2065

Merged

Conversation

yogeshlonkar
Copy link
Contributor

@yogeshlonkar yogeshlonkar commented Nov 21, 2024

GitLab has inconsistent naming of the filed for hidden variables.
It seems for keeping UI consistant, GitLab chose to introduce different
naming for the same field. This could have been easy handled in UI by
setting two variables on API request instead of introducing combined
field for UI.

I have added 2 test cases for create and update project API as per the
gitlab.com API that I manually tested and added similar unit tests.

@timofurrer timofurrer self-requested a review November 21, 2024 19:18
Copy link
Contributor

@timofurrer timofurrer left a comment

Choose a reason for hiding this comment

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

@yogeshlonkar thanks for the contribution 🤝 I've left some clarification questions on the correctness of the field / field naming. Back to you 🏓

project_variables.go Outdated Show resolved Hide resolved
project_variables.go Outdated Show resolved Hide resolved
@yogeshlonkar yogeshlonkar changed the title Add hidden field to ProjectVariable struct Add masked_and_hidden field to project create and get api Dec 3, 2024
@yogeshlonkar yogeshlonkar changed the title Add masked_and_hidden field to project create and get api Add masked_and_hidden field to project variable create and get api Dec 3, 2024
Copy link
Contributor

@RicePatrick RicePatrick left a comment

Choose a reason for hiding this comment

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

I went ahead and tested this against a locally running GitLab with the 2 suggested changes, and the tests pass. If you'd please make those changes, I'm good to merge.

project_variables.go Outdated Show resolved Hide resolved
project_variables.go Outdated Show resolved Hide resolved
@RicePatrick
Copy link
Contributor

Here is the test I used to test the above 2 suggestions:

func TestProjectVariablesService_MaskAndHidden_RealGitLab(t *testing.T) {
	client, err := NewClient("<token>",
		WithBaseURL("http://localhost:8085"),
	)
	if err != nil {
		t.Fatalf("Failed to create client: %v", err)
	}
	// create a project
	project, _, err := client.Projects.CreateProject(&CreateProjectOptions{
		Name: Ptr(fmt.Sprintf("test-%d", time.Now().UnixMicro())),
	})
	if err != nil {
		t.Fatalf("Failed to create project: %v", err)
	}
	projectVar, _, err := client.ProjectVariables.CreateVariable(project.ID, &CreateProjectVariableOptions{
		Key:             Ptr("test"),
		Value:           Ptr("testtesttest"), // must have a min of 8 characters
		MaskedAndHidden: Ptr(true),
	})
	if err != nil {
		t.Fatalf("Failed to create project variable: %v", err)
	}

	assert.True(t, projectVar.Hidden)
}

GitLab has inconsistent naming of the filed for hidden variables.
It seems for keeping UI consistant, GitLab chose to introduce different
naming for the same field. This could have been easy handled in UI by
setting two variables on API request instead of introducing combined
field for UI.

I have added 2 test cases for create and update project API as per the
gitlab.com API that I manually tested and added similar unit tests.
@yogeshlonkar yogeshlonkar changed the title Add masked_and_hidden field to project variable create and get api Add support for hidden field to project variables Dec 8, 2024
@yogeshlonkar
Copy link
Contributor Author

@RicePatrick @timofurrer can you please review PR again

@RicePatrick
Copy link
Contributor

Updated code looks good; merging so it's available for the migration tomorrow :)

@RicePatrick RicePatrick removed the request for review from timofurrer December 9, 2024 22:15
@RicePatrick RicePatrick dismissed timofurrer’s stale review December 9, 2024 22:16

Changes made as part of further commits

@RicePatrick RicePatrick merged commit 5626c64 into xanzy:main Dec 9, 2024
3 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants