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 certificate profile #106

Closed
wants to merge 1 commit into from
Closed

Conversation

ooraini
Copy link

@ooraini ooraini commented Feb 1, 2022

Notes:
Changing the profile will not delete and re-issue the certificate.
I'm not sure how would I write a test with the self-signed provider. I have tested it with IPA and it works as expected.

@richm
Copy link
Collaborator

richm commented Feb 1, 2022

[citest]

@richm richm mentioned this pull request Feb 1, 2022
@richm
Copy link
Collaborator

richm commented Feb 1, 2022

please rebase on top of latest master branch

@ooraini
Copy link
Author

ooraini commented Feb 1, 2022

@richm Done.

@richm
Copy link
Collaborator

richm commented Feb 1, 2022

[citest commit:e9187301a2a16ff54bebb7b7065d6bd9fa6d771d]

@richm
Copy link
Collaborator

richm commented Feb 1, 2022

Notes: Changing the profile will not delete and re-issue the certificate.

That sounds like the right behavior.

I'm not sure how would I write a test with the self-signed provider.

It sounds like this feature isn't applicable to self-signed provider?

I have tested it with IPA and it works as expected.

There is a test for ipa - https://github.com/linux-system-roles/certificate/blob/master/tests/tests_basic_ipa.yml#L13 - can you add a profile to one of these requests?

@richm
Copy link
Collaborator

richm commented Feb 1, 2022

[citest commit:b7d1b659eacf1ba2b014c138fb1a161fe283529d]

@ooraini ooraini force-pushed the master branch 2 times, most recently from 7a3e9c4 to 2e75c67 Compare February 1, 2022 19:04
@richm
Copy link
Collaborator

richm commented Feb 1, 2022

[citest commit:2e75c67c125946f363a9e6231c7c14d701c356e8]

@ooraini
Copy link
Author

ooraini commented Feb 1, 2022

This is disappointing. I will try to get things running locally and push again. Apologies.

@richm
Copy link
Collaborator

richm commented Feb 1, 2022

This is disappointing. I will try to get things running locally and push again. Apologies.

Looks like your ide/editor is not putting a newline at EOF?

@richm
Copy link
Collaborator

richm commented Feb 1, 2022

https://linux-system-roles.github.io/contribute.html - so you can use tox -e yamllint to check - just be aware that if you are using fedora, you will need to install tox using pip - pip install tox --user - then install the tox-lsr plugin - because the Fedora RPM tox package does not allow user plugins

the contribute page also has instructions about how to run tests locally using qemu/kvm

@ooraini ooraini force-pushed the master branch 2 times, most recently from f75dae1 to fbabc95 Compare February 4, 2022 06:19
@ooraini
Copy link
Author

ooraini commented Feb 4, 2022

[citest commit:fbabc95a3771b1d3698970d130e36f20a83313a0]

1 similar comment
@richm
Copy link
Collaborator

richm commented Feb 4, 2022

[citest commit:fbabc95a3771b1d3698970d130e36f20a83313a0]

@ooraini
Copy link
Author

ooraini commented Feb 5, 2022

Hi @richm,

The last CI failure was due to RHEL/CentOS 7 not outputting the profile in getcert list. The new test is quite brittle. I think that we might be better off without it. What do you think?

@ooraini ooraini force-pushed the master branch 2 times, most recently from d293ce7 to 8b9c586 Compare February 5, 2022 20:13
@richm
Copy link
Collaborator

richm commented Feb 7, 2022

Hi @richm,

The last CI failure was due to RHEL/CentOS 7 not outputting the profile in getcert list. The new test is quite brittle. I think that we might be better off without it. What do you think?

Can the test be made less brittle? We need some way to test this functionality (in addition, the test serves as a form of documentation for end users who may be trying to use this feature, and want to see a working example).

If there really is no way to test this functionality in an automated way with our current test framework, then we will need some detailed documentation about how a QE team can verify and validate the feature.

@richm
Copy link
Collaborator

richm commented Feb 7, 2022

[citest commit:8b9c5868eb3199b188c5b7814e3f4df0165d8455]

@ooraini
Copy link
Author

ooraini commented Feb 8, 2022

@richm I think this ready now. Not sure why is the CI is still pending.

@richm
Copy link
Collaborator

richm commented Feb 8, 2022

@richm I think this ready now.

yeah - lgtm - @rjeffman

Not sure what is why the CI is still pending.

fixed

@rjeffman rjeffman linked an issue Feb 10, 2022 that may be closed by this pull request
@richm
Copy link
Collaborator

richm commented Feb 14, 2022

please rebase the PR on top of the latest master branch

@richm
Copy link
Collaborator

richm commented Mar 17, 2022

please rebase the PR on top of the latest master branch

Resolves: linux-system-roles#88

Signed-off-by: Omar Aloraini <[email protected]>
@ooraini
Copy link
Author

ooraini commented Apr 4, 2022

@richm rebased.

@rjeffman
Copy link
Collaborator

rjeffman commented Apr 4, 2022

@ooraini and @richm, I'll be running a few tests against this PR today and tomorrow. On a first look, it looks good.

@richm
Copy link
Collaborator

richm commented Apr 4, 2022

[citest commit:d55578fe212cbb03c1edfbbf391459d37ff2f4f6]

@richm
Copy link
Collaborator

richm commented May 10, 2022

ping

@richm
Copy link
Collaborator

richm commented Aug 1, 2024

closing due to inactivity

@richm richm closed this Aug 1, 2024
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.

RFE - Add support for IPA Certificate Profiles
3 participants