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

Feature/venom fingerbank proxy #6786

Merged
merged 17 commits into from Dec 22, 2021
Merged

Feature/venom fingerbank proxy #6786

merged 17 commits into from Dec 22, 2021

Conversation

julsemaan
Copy link
Collaborator

Description

Test the Fingerbank proxy feature in venom

Impacts

Venom tests

Delete branch after merge

YES

@julsemaan julsemaan added this to the PacketFence-11.2 milestone Dec 20, 2021
@julsemaan julsemaan requested a review from nqb December 20, 2021 20:19
@julsemaan
Copy link
Collaborator Author

julsemaan commented Dec 20, 2021

One thing I'm wondering is should this be part of the configurator scenario?

Currently, its in its own scenario

@julsemaan
Copy link
Collaborator Author

I ran this successfully on runner2 but a pipeline is currently running to assess it works in the official CI environment: https://gitlab.com/inverse-inc/packetfence/-/pipelines/433611476

@nqb
Copy link
Contributor

nqb commented Dec 21, 2021

One thing I'm wondering is should this be part of the configurator scenario?

No, this scenario is specific and only used for the release.

IMHO, it's better to have a dedicated scenario because we boot an additional VM (linux02 here). I suggest to rename your scenario to something more generic related to external services provided by other VM. We will likely reused that scenario to add more tests which cover integration like syslog forwarding, RADIUS, etc.

Copy link
Contributor

@nqb nqb left a comment

Choose a reason for hiding this comment

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

  • For test cases name, could you use something like name_of_my_testcase like get_login_token in place of sentences with spaces ?
  • Could you add a TESTSUITE.md in fingerbank_proxy which describe logic of the test (with custom iptables rules, test can be run offline, etc.). You can find an example here.
  • An improvement could be to create a new test suite like configurator_proxy to configure Fingerbank proxy at this step.

@julsemaan
Copy link
Collaborator Author

I've addressed most of the comments except the job name

@nqb, in your opinion, how should we name the scenario that is proxy centric (or linux02 centric)?

And do we rename the scenario, make entry and gitlab entry ? or just the make+gitlab entry and run multiple scenarios in that make entry?

@nqb
Copy link
Contributor

nqb commented Dec 21, 2021

@nqb, in your opinion, how should we name the scenario that is proxy centric (or linux02 centric)?

I would suggest something related to external integrations (nothing prevent us to boot ad VM later) . external_integrations as scenario name looks good to me. You need to rename gitlab entry, Makefile entry and name of directory into scenarios directory. You can keep your test suites as is.

@julsemaan
Copy link
Collaborator Author

New pipeline with adjustments: https://gitlab.com/inverse-inc/packetfence/-/pipelines/434354693

Now we just need to state on if the strategy for testing the Fingerbank connectivity is acceptable or not

@nqb
Copy link
Contributor

nqb commented Dec 22, 2021

@julsemaan, thanks for your adjustments.

I think we are good to merge based on answers you provided related to Fingerbank Connectivity.

@nqb nqb merged commit 289863b into devel Dec 22, 2021
nqb added a commit that referenced this pull request Dec 22, 2021
nqb added a commit that referenced this pull request Dec 22, 2021
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.

None yet

2 participants