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

Adds the ability on render to deleted targeted files or directories #1073

Merged
merged 5 commits into from
Feb 18, 2022

Conversation

costrouc
Copy link
Member

Closes #1049

This is to provide a simple mechanism for deprication of files and
allowing for targeted deletion of files. Later this functionality can
be extented to provide qhub extensions the ability to depricate files
within a stage. For how this is just a single list that is populated.

Closes #1049

This is to provide a simple mechanism for deprication of files and
allowing for targeted deletion of files. Later this functionality can
be extented to provide qhub extensions the ability to depricate files
within a stage. For how this is just a single list that is populated.
Copy link
Contributor

@tonyfast tonyfast left a comment

Choose a reason for hiding this comment

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

looks like it will do the trick, but i'm slightly concerned deleted_files might be a confusing name. deleted raises 🎏 and deprecated_files or deprecated_conventions has a softer blow.

".terraform",
"__pycache__",
],
deleted_paths=[
Copy link
Contributor

@tonyfast tonyfast Feb 18, 2022

Choose a reason for hiding this comment

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

i think i'm worried that defining these paths deep in the code might wake it hard to manage these deprecations. it seems that this is code style as it is. later, we might want to tear this out into a deprecate.py file or something.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the review @tonyfast. What do you think about the recent additions? I've added an explicit DEPREICATED_FILE_PATHS constant within qhub/depricate.py. Eventually I'd like to add this logic to our extension mechanism to allow for stages to depricate files.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd like to keep the deleted_paths bit just to communicate that some files will actually be deleted.

Copy link
Contributor

@viniciusdc viniciusdc left a comment

Choose a reason for hiding this comment

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

All right, the deletion logic has no issues. LGTM! Thanks @costrouc

Copy link
Contributor

@tonyfast tonyfast left a comment

Choose a reason for hiding this comment

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

the approach looks sound. we should fix the spelling of deprecate before merging.

@costrouc
Copy link
Member Author

Made changes and need to wait for CI to pass

@costrouc costrouc requested a review from danlester February 18, 2022 17:07
@costrouc
Copy link
Member Author

@danlester would you mind reviewing this if it does all you need in #1049

@danlester
Copy link
Contributor

This is certainly capable of fixing the problems we've seen, but is probably overkill in terms of expecting developers to use this system mid-release - which is where we've encountered recent problems.

Ultimately, the advice for developers is to clear everything out periodically.

But we will be able to use this if we keep track of files changed between releases, to avoid the same problems out in the wild.

@costrouc
Copy link
Member Author

This is certainly capable of fixing the problems we've seen, but is probably overkill in terms of expecting developers to use this system mid-release - which is where we've encountered recent problems.

Ultimately, the advice for developers is to clear everything out periodically.

Yeup I agree. It's also go practice for us to delete the directory to make sure that the deployment does not depend on the files and hidden state.

This PR is there to address issues that user's of qhub will face and make as minimal deletions as possible.

@costrouc costrouc merged commit 9fb62c8 into main Feb 18, 2022
@costrouc costrouc deleted the fix-1049 branch February 18, 2022 21:03
tylerpotts pushed a commit that referenced this pull request Feb 21, 2022
…1073)

* Adds the ability on render to deleted targeted files or directories

Closes #1049

This is to provide a simple mechanism for deprication of files and
allowing for targeted deletion of files. Later this functionality can
be extented to provide qhub extensions the ability to depricate files
within a stage. For how this is just a single list that is populated.

* Black formatting

* Adding qhub/depricate.py to address explicit deprication of files

* Black formatting

* Need to learn to spell 😄
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.

[enhancement] clear out stages before re-render
4 participants