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

ci: add job and script for rdmo-app check #1

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

MyPyDavid
Copy link
Member

Ive made a bash script for this job but it probably needs to be converted into the Action ci.yml .
Hope it runs 🤞

@MyPyDavid MyPyDavid self-assigned this Feb 1, 2024
@MyPyDavid
Copy link
Member Author

MyPyDavid commented Feb 1, 2024

wanna review this as well @afuetterer?🙏
I can't select you from the Reviewers user list

python manage.py check

# function for testing presence of plugin name in a certain django setting
test_if_settings_contain_plugin () {
Copy link
Member Author

Choose a reason for hiding this comment

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

thank you @triole! Ive included your suggestions and refactored the last part into a function. Looks good to you?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, this is just testing that the plugin is in settings which is rather pointless, since the settings are part of the test setup. I think a simple ./manage.py check would suffice, right?

@afuetterer
Copy link
Member

wanna review this as well @afuetterer?🙏
I can't select you from the Reviewers user list

You got this. Glad you took it up to add some tests here.

@MyPyDavid
Copy link
Member Author

shall we merge this @jochenklar ?


git clone https://$GITHUB_TOKEN@github.com/rdmorganiser/rdmo.git
# need rdmo only for testing/config/settings
git clone https://$GITHUB_TOKEN@github.com/rdmorganiser/rdmo-app.git
Copy link
Member

Choose a reason for hiding this comment

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

I think the pluging can be tested in rdmo/testing/ alone, right? So cloning rdmo (no token needed) woulsd suffice.

python manage.py check

# function for testing presence of plugin name in a certain django setting
test_if_settings_contain_plugin () {
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, this is just testing that the plugin is in settings which is rather pointless, since the settings are part of the test setup. I think a simple ./manage.py check would suffice, right?

Copy link
Member

Choose a reason for hiding this comment

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

This part should not be part of the repo. If it is only needed for testing it should just be part of the ci job.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe this can just be solved with some echo > magic and multiline yaml fields >-.

@jochenklar
Copy link
Member

I would also like it better if the bash part would just be part of the yaml file. That makes it much easiert to see what the ci is doing. It is also easiert to move the script to other plugins.

@MyPyDavid
Copy link
Member Author

I think that actually this whole CI thing should go into its own repo, eg. rdmo-plugins-gh-action-check so that this action only needs to be imported by the ci.yml of each plugin and not be copy-pasted for each one..

@jochenklar
Copy link
Member

Yes! I have not thought about it. Please go ahead if you can/want to work on it. I am curious how these things are implemented.

@afuetterer
Copy link
Member

I would like to propose a solution for this as well.

I can work on the setup. But how to test this plugin actually works? What is a unit or integration test? Is the desired output the rendered dataset?

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.

4 participants