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

Generate newer version of test-info.yml #96

Merged
merged 4 commits into from
May 8, 2024

Conversation

spuiuk
Copy link
Collaborator

@spuiuk spuiuk commented May 3, 2024

The new generation of tests in sit-test-cases require the use of the new version of test-info.yml files.

Changing the template to generate the new version of test-info.yml files.

@spuiuk spuiuk requested review from xhernandez and anoopcs9 May 3, 2024 19:45
@spuiuk spuiuk force-pushed the testinfo branch 3 times, most recently from 8de5eda to 6b67d05 Compare May 5, 2024 22:42
Copy link
Collaborator

@xhernandez xhernandez left a comment

Choose a reason for hiding this comment

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

The change looks fine, but I think that the test-info.yml creation should be moved inside test.sit-test-cases role, since it's specific to that test. I'm ok if this is done in a future patch, though.

spuiuk added 4 commits May 7, 2024 22:12
This isn't used in testing.

Signed-off-by: Sachin Prabhu <[email protected]>
Use the new test-info.yml file as required by sit-test-cases.

Signed-off-by: Sachin Prabhu <[email protected]>
Access to the backend filesystem is already provided through sshfs. Make
sure that this is listed in test-info.yml allowing the testing
infrastructure to prepare the backend for a test.

Signed-off-by: Sachin Prabhu <[email protected]>
@spuiuk
Copy link
Collaborator Author

spuiuk commented May 8, 2024

/retest all

- name: Create the test-info.yml file with test cluster information
template:
src: test-info.yml.j2
dest: /root/test-info.yml
Copy link
Collaborator

Choose a reason for hiding this comment

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

You could put it directly in /root/sit-test-cases/test-info.yml and avoid the symlink creation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am trying to remember why I did it like this. I think I did it for re-use. ie. in case other testing projects wanted to make use of the information in that config file.
Another advantage is in a devel environment where we want to delete the whole sit-test-cases repo and redo the folder, it is easier if the configuration is outside the directory.

If it isn't too much of an issue, can we leave it as it is?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe it was a simple case of the repo being pulled after the test-info.yml file was created and hence the conf file wasn't created inside the repo . :) but the points above still hold.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am trying to remember why I did it like this. I think I did it for re-use. ie. in case other testing projects wanted to make use of the information in that config file.

The contents of the file are very sit-test-cases specific (that's why I suggested moving it inside the test.sit-test-cases role), so it's unlikely that it will be reused by other tests. Each test can create it's own custom configuration if required.

Another advantage is in a devel environment where we want to delete the whole sit-test-cases repo and redo the folder, it is easier if the configuration is outside the directory.

You still need to re-run the playbook to recreate the symlink. It's true that you can do it manually, though.

If it isn't too much of an issue, can we leave it as it is?

Yes, it's not a big problem, and it's contained all inside the test.sit-test-cases role, so if you prefer to keep it this way, I'm ok with that.

@spuiuk spuiuk merged commit fb92a59 into samba-in-kubernetes:main May 8, 2024
9 checks passed
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.

2 participants