Add an add-admin action

[nrobinaubertin]

The goal is to add a add-admin action to the charm that uses a modified version of indico CLI tool.
I added a click command group (via plugin) to allow the indico autocreate admin <email> <password> command.

[jdkandersson]

Haven't finished this review yet, there are some broad questions about whether this should be a CLI or it should be in the charm source code. Let's address that first and then I'll finish the review

[nrobinaubertin]

Haven't finished this review yet, there are some broad questions about whether this should be a CLI or it should be in the charm source code. Let's address that first and then I'll finish the review

Sorry, it probably wasn't obvious from the PR description. I'm patching a file from the source code of indico (here is the source).
I decided to do it that way to avoid fiddling with their internal database structure.

So I want to do it in a way that is simple, ideally robust to some unrelated changes in indico and better, that breaks when there are changes that would affect this change.
I thought of 3 ways to do it:

1. wget the source and adding my bit at the end
2. wget the source and apply a git patch
3. replacing the whole file

I decided to replace the whole file (seemed simpler, but maybe not). But doing that, I find it better to avoid touching the code that the indico team/community wrote around the block that I'm adding. Eventually, I plan to make a PR on indico's side

[jdkandersson]

Haven't finished this review yet, there are some broad questions about whether this should be a CLI or it should be in the charm source code. Let's address that first and then I'll finish the review

Sorry, it probably wasn't obvious from the PR description. I'm patching a file from the source code of indico (here is the source). I decided to do it that way to avoid fiddling with their internal database structure.

So I want to do it in a way that is simple, ideally robust to some unrelated changes in indico and better, that breaks when there are changes that would affect this change. I thought of 3 ways to do it:

1. wget the source and adding my bit at the end
2. wget the source and apply a git patch
3. replacing the whole file

I decided to replace the whole file (seemed simpler, but maybe not). But doing that, I find it better to avoid touching the code that the indico team/community wrote around the block that I'm adding. Eventually, I plan to make a PR on indico's side

So are we making any modifications from that source or is it just vending in that file? with no changes?

[nrobinaubertin]

So are we making any modifications from that source or is it just vending in that file? with no changes?

I'm adding the block with the command "autocreate"

indico-operator/files/src/indico/cli/user.py

Line 195 in 4bc932f

@cli.command("autocreate")

[jdkandersson]

So are we making any modifications from that source or is it just vending in that file? with no changes?

I'm adding the block with the command "autocreate"

indico-operator/files/src/indico/cli/user.py

Line 195 in 4bc932f

@cli.command("autocreate")

Would it be possible to move that to the charm source code?

[gregory-schiano]

Haven't finished this review yet, there are some broad questions about whether this should be a CLI or it should be in the charm source code. Let's address that first and then I'll finish the review

Sorry, it probably wasn't obvious from the PR description. I'm patching a file from the source code of indico (here is the source). I decided to do it that way to avoid fiddling with their internal database structure.

So I want to do it in a way that is simple, ideally robust to some unrelated changes in indico and better, that breaks when there are changes that would affect this change. I thought of 3 ways to do it:

1. wget the source and adding my bit at the end
2. wget the source and apply a git patch
3. replacing the whole file

I decided to replace the whole file (seemed simpler, but maybe not). But doing that, I find it better to avoid touching the code that the indico team/community wrote around the block that I'm adding. Eventually, I plan to make a PR on indico's side

The risk here is that upgrading indico version could lead to incompatibility. Which means we'll have to pay attention to port the change on that file everytime we update indico.
A patch is a bit more flexible but still not ideal
Ideally we should submit this change to indico project to have it as part of the project itself.
Wouldn't it be possible to add an extra file independent that adds this new command ?

[nrobinaubertin]

Wouldn't it be possible to add an extra file independent that adds this new command ?

I thought of adding a new module in its own file. But I still have to load it therefore change the core.py file of indico's CLI

[gregory-schiano]

Wouldn't it be possible to add an extra file independent that adds this new command ?

I thought of adding a new module in its own file. But I still have to load it therefore change the core.py file of indico's CLI

After investigating and discussing with Niels, Indico supports plugins that can extend CLI
I suggested to have a look at this as it will be less intrusive and more generic

[jdkandersson]

Any integration tests?

[arturo-seijas]

Can you update the PR description to match the current action params?

[amandahla]

Wouldn't it be possible to add an extra file independent that adds this new command ?

I thought of adding a new module in its own file. But I still have to load it therefore change the core.py file of indico's CLI

After investigating and discussing with Niels, Indico supports plugins that can extend CLI I suggested to have a look at this as it will be less intrusive and more generic

Just to add another option to consider, it could be possible to pass input to the "indico user create --admin" command using expect:
https://phoenixnap.com/kb/linux-expect

So wouldn't be necessary to change anything in Indico.

[nrobinaubertin]

Just to add another option to consider, it could be possible to pass input to the "indico user create --admin" command using expect

Yes, this was considered during the discussions that lead to a conclusion to this thread.
The answer of David was that even using that way of doing things would call for more e2e testing.

[jdkandersson]

Looking good, still seems to be some issues with the lint command

Thanks for your resilience with all the feedback!

[nrobinaubertin]

Looking good, still seems to be some issues with the lint command

The issue here is that the lint tests are running on focal (python 3.8) and the pypi package for indico 3.2 is not compatible with it.
I think that the best course of action is to remove the plugin from the charm lint tests and move it to a section just for it.

[github-actions]

Test coverage for d7db4c4

Name           Stmts   Miss Branch BrPart  Cover   Missing
----------------------------------------------------------
src/charm.py     303      6     82     12    95%   124->exit, 140, 144, 497->501, 498->exit, 545->548, 648->687, 765-766, 828->exit, 848->864, 850->859, 859->864, 872-873, 916->exit
----------------------------------------------------------
TOTAL            303      6     82     12    95%

Static code analysis report

Run started:2023-02-03 16:23:13.101669

Test results:
  No issues identified.

Code scanned:
  Total lines of code: 1801
  Total lines skipped (#nosec): 4
  Total potential issues skipped due to specifically being disabled (e.g., #nosec BXXX): 0

Run metrics:
  Total issues (by severity):
  	Undefined: 0
  	Low: 0
  	Medium: 0
  	High: 0
  Total issues (by confidence):
  	Undefined: 0
  	Low: 0
  	Medium: 0
  	High: 0
Files skipped (0):

[arturo-seijas]

LGTM

[amandahla]

LGTM

[jdkandersson]

The integration tests failed...

[jdkandersson]

If the lint checks fail for Ubuntu 20.04, does the add-admin action actually work on Python 3.8? It looks like it wasn't possible to install indico on Python 3.8?