Skip to content

Introduce cruft to keep the repository up to date with template changes #144

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

Closed
wants to merge 15 commits into from

Conversation

Kircheneer
Copy link
Collaborator

@Kircheneer Kircheneer commented Jul 27, 2022

Why is this not in the CI (already)? I wasn't sure about including it because that would mean that updates to the cookiecutter template would cause this CI to somewhat arbitrarily fail, we should follow up at a later point, maybe post diffs to slack if there are any?

@Kircheneer Kircheneer marked this pull request as ready for review July 27, 2022 11:29
Copy link
Contributor

@chadell chadell left a comment

Choose a reason for hiding this comment

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

awesome! 🎉

chadell
chadell previously approved these changes Jul 27, 2022
Copy link
Contributor

@chadell chadell left a comment

Choose a reason for hiding this comment

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

LGTM

about: Report a reproducible bug in the current release of DiffSync
about: Report a reproducible bug in the current release of diffsync
Copy link
Collaborator

Choose a reason for hiding this comment

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

DiffSync is the project name, diffsync is the Python package name. I'd prefer to keep it as DiffSync when we're referring to the project as a whole.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed. We need to address this at the source, because currently this is cruft working as intended.

@@ -0,0 +1,29 @@
---
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this a separate file just so as to keep ci.yml identical with the cookiecutter? I would have hoped that it'd be possible/straightforward with cruft to have a locally modified version of the file (i.e. some consistent diff against the baseline). This feels like a bit of a step backwards to me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unfortunately this doesn't seem to be possible right now in cruft. I agree that this is worse, because now we can't cross-reference between the two sets of workflows. Possibly this is something that we can address by using another paradigm in GitHub actions.

Comment on lines -186 to +216
.idea/
# User-specific stuff
.idea/**/workspace.xml
.idea/**/tasks.xml
.idea/**/usage.statistics.xml
.idea/**/dictionaries
.idea/**/shelf

# Generated files
.idea/**/contentModel.xml

# Sensitive or high-churn files
.idea/**/dataSources/
.idea/**/dataSources.ids
.idea/**/dataSources.local.xml
.idea/**/sqlDataSources.xml
.idea/**/dynamic.xml
.idea/**/uiDesigner.xml
.idea/**/dbnavigator.xml

# Gradle
.idea/**/gradle.xml
.idea/**/libraries
Copy link
Collaborator

Choose a reason for hiding this comment

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

This feels like a step backwards to me. We should continue to ignore all of .idea/ instead of this laundry list of files.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it comes from Cookiecutter, we should fix first there :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

If cruft prevents us from improving things locally as compared to the cookiecutter baseline, that feels like a non-starter to me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed. This also needs to be addressed at the template level.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

And to your comment Glenn, we can always ignore individual files that we do not want to keep in sync in the pyproject.toml file. I do agree that this is rather heavy-handed but since currently we do not have any automatic way of keeping downstream files in sync I believe that this should still be an improvement over the current state, what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

If cruft prevents us from improving things locally as compared to the cookiecutter baseline, that feels like a non-starter to me.

My overall approach would be, cruft helps to adhere to the cookiecutter. I we do an improvement here (we deviate), cruft should raise a warning, and we should focus on change the cookiecutter reference to include the improvement.
I mean, when improving, deviations are expected, and this should help to keep in the right track (not enforcing)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That is an interesting idea, we will need to hash out the corresponding process further

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To give a quick update here, there is an issue that mentioned the ability to skip specific patches, it is closed however because another part of the issue was implemented. I do believe that this feature specifically wasn't implemented, but I have gone ahead and asked that here.

@@ -5,6 +5,12 @@ FROM python:${PYTHON_VER}-slim
RUN pip install --upgrade pip \
&& pip install poetry

RUN apt-get update && apt-get install -y \
gcc \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Adding gcc feels pretty heavyweight for this container - what's it needed for?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This comes from psutil, I believe in conjunction with Buildx:

#10 5.989         C compiler or Python headers are not installed on this system. Try to run:
#10 5.989         sudo apt-get install gcc python3-dev
#10 5.989         error: command 'gcc' failed: No such file or directory: 'gcc'
#10 5.989         [end of output]

@Kircheneer
Copy link
Collaborator Author

Closing this for now, the interesting bits were cherry-picked into #176. Will re-evaluate approach and reconvene later.

@Kircheneer Kircheneer closed this Nov 1, 2022
@jdrew82 jdrew82 deleted the lk-cruft branch March 27, 2025 22:36
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.

Re-evaluate pytest-redislite as a dependency
3 participants