SUSE / velum

Dashboard for CaaS Platform clusters (v1, v2 and v3)
https://www.suse.com/
Apache License 2.0
54 stars 30 forks source link

Introduce admission webhook settings page #590

Open flavio opened 6 years ago

flavio commented 6 years ago

Allow user to enable AdmissionWebhook by using a new dedicated page.

To work this requires the following PR merged into the salt repo: https://github.com/kubic-project/salt/pull/555

The new UI has to be improved by @vitoravelino, right now it looks something like that:

image

The workflow is the following one:

Notable things missing:

vitoravelino commented 6 years ago

Current state with last commit:

screenshot-20180619174743-431x277 screenshot-20180619174814-670x444 screenshot-20180619174851-673x755

vitoravelino commented 6 years ago

@flavio This is good to be reviewed.

flavio commented 6 years ago

It looks to me, I like the workflow. However there's a bug:

Basically we don't actually delete them from the database.

flavio commented 6 years ago

Also another bug: it's possible to upload empty files. The validation in place doesn't look for that

vitoravelino commented 6 years ago

Fixed both cases, @flavio. Thanks!

vitoravelino commented 6 years ago

screenshot-20180622141838-295x229

flavio commented 6 years ago

I cannot be the one approving the PR because I'm the one who created it. But it LGTM

vitoravelino commented 6 years ago

I guess we can proceed with the merge, right? I'll give my approval on behalf of @flavio since he created the PR and can't do that.

vitoravelino commented 6 years ago

I updated the code with with the x509 validation and added pem/der key format validation. Suggestion for wording are welcome. I also made a minor refactoring to make things more readable.

Please review it again. Thanks!

@flavio, could you play with this again?

bergmannf commented 6 years ago

Just a short update to fix the merge conflicts. I still intend to setup an admission webhook and actually try out the code.

vitoravelino commented 6 years ago

Just a short update to fix the merge conflicts. I still intend to setup an admission webhook and actually try out the code.

I don't see any conflicts. Is something going to change?

bergmannf commented 6 years ago

Sorry - I just meant I repushed the changes after a rebase due to conflicts in app/controllers/internal_api/v1/pillars_controller.rb - the dex connector apparently added some more pillars.

vitoravelino commented 6 years ago

I've rebased this and should be good to be reviewed again.

bergmannf commented 6 years ago

LGTM with one small change (use fixture_file_upload).

However given that the salt PR is on hold I don't think we can merge this right now.

MalloZup commented 5 years ago

This GitHub PR is unactive since 30. Is this GitHub PR still needed? Please close or update it accordingly. This reminder is autogenerated by https://github.com/MalloZup/blacktango