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

Enable completion tracking by default #354

Merged
merged 3 commits into from
Oct 29, 2024
Merged

Conversation

mikix
Copy link
Contributor

@mikix mikix commented Oct 28, 2024

  • Removes the --write-completion opt-in flag, it is now always enabled.
  • Requires export group name and timestamp information to be available, either from export log or from CLI.
  • Updates some user docs, explaining how completion tracking expects to be fed data.
  • Drops deprecated subcommand alias chart-review (old name for upload-notes)
  • Bump pyarrow to 18.x (which allows compatibility with python 3.13)
  • Bumps version to 2.0.0
  • Tag docker releases with major number, so that someone who wants to be careful to avoid unexpected CLI breaks like this introduces could track their install that way.

This is the last big piece of #296

Checklist

  • Consider if documentation (like in docs/) needs to be updated
  • Consider if tests should be added

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file added so that the "sample ETL runs" in our docs, which point at this folder, can continue working.

@mikix mikix force-pushed the mikix/completion-by-default branch from b0a482b to 5ad9a7e Compare October 28, 2024 19:10
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file's changes are not yet tested - let's hope it works? Else I can do some fix up PRs afterward. Not sure to best test this piece of the deployment mechanism (without affecting existing users of our docker images)

Copy link
Contributor

Choose a reason for hiding this comment

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

you could temporarily change the tagging strategy? then you could run it from this branch without disruption?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did this - and it did catch an issue, so good idea to push back against my yolo strategy 😄 - filed the docker stuff separately though in #355

Copy link

github-actions bot commented Oct 28, 2024

☂️ Python Coverage

current status: ✅

Overall Coverage

Lines Covered Coverage Threshold Status
3538 3477 98% 98% 🟢

New Files

No new covered files...

Modified Files

File Coverage Status
cumulus_etl/init.py 100% 🟢
cumulus_etl/cli.py 100% 🟢
cumulus_etl/errors.py 100% 🟢
cumulus_etl/etl/cli.py 100% 🟢
cumulus_etl/etl/tasks/base.py 100% 🟢
cumulus_etl/etl/tasks/basic_tasks.py 100% 🟢
TOTAL 100% 🟢

updated for commit: ef7e6f4 by action🐍

@mikix mikix marked this pull request as ready for review October 28, 2024 19:28
Copy link
Contributor

Choose a reason for hiding this comment

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

you could temporarily change the tagging strategy? then you could run it from this branch without disruption?

Copy link
Contributor

Choose a reason for hiding this comment

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

coherence check - do we need to add any updates anyplace else? should we link to the expectations from sections of the docs like sample runs to call out some of the new assumptions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK yeah, I added a link to this bulk export doc from the sample run doc in the "more realistic command" section, like "and if you want real data, see this doc for how"

@mikix mikix force-pushed the mikix/completion-by-default branch 4 times, most recently from 5a3ca1e to 6982a60 Compare October 29, 2024 13:56
This allows us to build on Python 3.13 - I've tested that we pass tests
on that version as well (i.e. we don't seem to be affected by the
deprecation removal in 3.13)
And remove deprecated chart-review alias for upload-notes, since this
is a good time to break CLI promises.
- Removes the --write-completion opt-in flag, it is now always enabled.
- Requires export group name and timestamp information to be available,
  either from export log or from CLI.
- Updates some user docs, explaining how completion tracking expects to
  be fed data.
@mikix mikix force-pushed the mikix/completion-by-default branch from 6982a60 to ef7e6f4 Compare October 29, 2024 14:32
@mikix mikix merged commit 1ae37fd into main Oct 29, 2024
3 checks passed
@mikix mikix deleted the mikix/completion-by-default branch October 29, 2024 14:57
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