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

Update README, migrate it to Markdown #235

Merged
merged 4 commits into from
Nov 23, 2024
Merged

Update README, migrate it to Markdown #235

merged 4 commits into from
Nov 23, 2024

Conversation

amercader
Copy link
Member

Reformatted the README as Markdown to make it easier to update. Did a full review and updated broken links and added config options to increase visibility (generated with ckan config docs --enabled -f md)

Reformatted the README as Markdown to make it easier to update. Did a
full review and updated broken links and added config options to
increase visibility (generated with `ckan config docs --enabled -f md`)
Copy link
Collaborator

@duttonw duttonw left a comment

Choose a reason for hiding this comment

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

Hi @amercader,

Thanks for the contribution in converting the readme from rst to markdown.

I've raise a couple of items we can improve on.

This is more on what was already existing and not about the work you have done so far.

If you don't have time to do these alterations. We can still accept this PR as is.

Regards,

@duttonw

README.md Show resolved Hide resolved
README.md Show resolved Hide resolved

3. Install dependencies:

pip install -r https://raw.githubusercontent.com/ckan/ckanext-xloader/master/requirements.txt
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's time this is cleaned up as using the latest instead of what you installed via pypi or git is not very good in the scheme of things.

For www.data.qld.gov.au, we use this bash like script to do the find of requirements to install.
see https://github.com/qld-gov-au/opswx-ckan-cookbook/blob/dde4e48ac6c50e663c364afec462224dcf7fc9a1/resources/pip_install_app.rb#L114C17-L134C66
(we may not need all of this but this is what we use to cover all plugins out in the wild atm)

PYTHON_MAJOR_VERSION=$(#{new_resource.virtualenv_dir}/bin/python -c "import sys; print(sys.version_info.major)")
PYTHON_REQUIREMENTS_FILE=requirements-py$PYTHON_MAJOR_VERSION.txt
if [ -f $PYTHON_REQUIREMENTS_FILE ]; then
	REQUIREMENTS_FILE=$PYTHON_REQUIREMENTS_FILE
else
	CKAN_MINOR_VERSION=$(#{new_resource.virtualenv_dir}/bin/python -c "import ckan; print(ckan.__version__)" | grep -o '^[0-9]*[.][0-9]*')
	CKAN_REQUIREMENTS_FILE=requirements-$CKAN_MINOR_VERSION.txt
	if [ -f "$CKAN_REQUIREMENTS_FILE" ]; then
		REQUIREMENTS_FILE=$CKAN_REQUIREMENTS_FILE
	else
		REQUIREMENTS_FILE=requirements.txt
	fi
fi
if [ -f "$REQUIREMENTS_FILE" ]; then
	REQUIREMENTS_FILES="-r $REQUIREMENTS_FILE"
fi
# ckanext-harvest uses this filename
if [ -f "pip-requirements.txt" ]; then
	REQUIREMENTS_FILES="$REQUIREMENTS_FILES -r pip-requirements.txt"
fi
pip install -e . $REQUIREMENTS_FILES || exit 1

Copy link
Member Author

Choose a reason for hiding this comment

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

@duttonw I agree installing the dependencies in master it's not ideal. But until dependencies are packaged as part of the published package and installed as part of pip install ckanext-xloader is perhaps the least confusing option with the suggested install method. Otherwise I'd suggest the clone, install editable, install requirements.txt approach that we use on other extensions:

git clone https://github.com/ckan/ckanext-xloader
cd ckanext-xloader
pip install -e .
pip install -r requirements.txt

If I'm not mistaken the script you shared deals with finding the right requirements file locally but not choosing the right one for a particular version of the extension

Copy link
Collaborator

Choose a reason for hiding this comment

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

If I'm not mistaken the script you shared deals with finding the right requirements file locally but not choosing the right one for a particular version of the extension

No, it should handle that. If the requirements are defined correctly, then checking out a specific commit should have requirements that match the code.

The point of the change is to be able to handle multiple external dependencies, eg different CKAN versions and/or Python versions.

Copy link
Member Author

Choose a reason for hiding this comment

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

Exactly, but the current suggested install method (pip install ckanext-xloader or pip install git+https://github.com/ckan/ckanext-xloader@master) won't check out the code in a place where the user or a script can find it (it will go to the site-packages folder of the venv), so we can not point them to an actual local requirements.txt to run pip install -r ...

I'm happy with whatever you think makes more sense, I just don't know how to translate this script into actual instructions.

To be honest, I think xloader would be fine defining its requirements in pyprojec.toml using the proposed guidelines as most of the pinned stuff in requirements.txt are CKAN core requirements anyway

Copy link
Collaborator

Choose a reason for hiding this comment

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

yep, Transition to pyproject.toml is on the cards. Am planning on borrowing how DataShades/ckanext-oidc-pkce@c027158 this project did it in the near future.

@duttonw
Copy link
Collaborator

duttonw commented Nov 21, 2024

please rebase as master now has a fix for the docker containers not being root any more which break said cicd build for 2.11.

@duttonw duttonw merged commit d08d61a into master Nov 23, 2024
10 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.

3 participants