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

Add variable certificate store columns #294

Merged
merged 3 commits into from
Dec 13, 2023
Merged

Add variable certificate store columns #294

merged 3 commits into from
Dec 13, 2023

Conversation

rhburt
Copy link
Contributor

@rhburt rhburt commented Dec 11, 2023

Closes #250

I've adapted how variable columns are accomplished in image list to work for config trust list.

Contributing guidelines said to update the translations alongside changes to any CLI strings as well.

@stgraber
Copy link
Member

 Error: cmd/incus/config_trust.go:496:2: S1011: should replace loop with `projects = append(projects, rowData.Cert.Projects...)` (gosimple)
	for _, project := range rowData.Cert.Projects {
	^

@stgraber
Copy link
Member

This is also breaking the TLS restrictions test, it likely just needs a simple update to select just the column it requires.

@stgraber stgraber added the Incomplete Waiting on more information from reporter label Dec 11, 2023
@stgraber
Copy link
Member

Hey there, you're likely going to want to add the description column now that it's been added with #297

@rhburt
Copy link
Contributor Author

rhburt commented Dec 12, 2023

Hey @stgraber

I've fixed both the gosimple error and the TLS restrictions test. I'm still getting a failing test from suites/basic.sh, but I've run the test over the main branch with the same result so I don't believe my changes are affecting that test. Please let me know if I'm wrong here.

I've also added in the description column as per your last comment. It appears the config trust add-certificate command does not support a description field, so I wasn't able to verify the change past ensuring the column exists in the table.

Thanks!

@stgraber
Copy link
Member

I've also added in the description column as per your last comment. It appears the config trust add-certificate command does not support a description field, so I wasn't able to verify the change past ensuring the column exists in the table.

incus config trust edit should let you set the description field.

@rhburt
Copy link
Contributor Author

rhburt commented Dec 12, 2023

I've also added in the description column as per your last comment. It appears the config trust add-certificate command does not support a description field, so I wasn't able to verify the change past ensuring the column exists in the table.

incus config trust edit should let you set the description field.

Great, thanks. I've confirmed the description is displaying as expected. I should have some time tonight to figure out why the test_remote_admin test is failing.

The suites/basic.sh failure turned out to be a permission error on my system, so no worries there.

@stgraber
Copy link
Member

@rhburt I took a quick look. I also used the opportunity to extract the test changes to a separate commit.

Basically the issue here was an invocation of incus config trust list in the previous test (remote_usage) which was meant to delete all certificates from the trust store, but didn't anymore due to the syntax change. This was then causing the next test (remote_admin) to run with a dirty state and break.

That kind of breakage can be a bit weird to debug as running the test in isolation wouldn't show the issue.

@stgraber stgraber removed the Incomplete Waiting on more information from reporter label Dec 12, 2023
@stgraber
Copy link
Member

Oops, sorry I got distracted by all of today's drama, PR looks great now!

Thanks!

@stgraber stgraber merged commit 76be151 into lxc:main Dec 13, 2023
25 checks passed
@rhburt rhburt deleted the cert-columns branch December 13, 2023 02:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Tweak certificate store columns
2 participants