Skip to content

Conversation

@eantyshev
Copy link
Contributor

@eantyshev eantyshev commented Nov 29, 2022

resolves #24225
follow-up PR which uses new fields: #24425

example tag:

# beam-playground:
#   name: setting-pipeline
#   description: Setting pipeline example.
#   url_notebook: https://some-url/py-file.ipynb
#   multifile: false
#   context_line: 35

Resulting pg_examples entity has an old path field along with new ones: urlVCS(==path) and urlNotebook

Proposed update sequence:

  1. backend deployed. New backend API provides all fields: link, url_vcs, and url_notebook to not break old frontend client
  2. examples CD step is run. Both backend versions, old and new consume different pg_examples fields
  3. frontend merged, deployed. New frontend consumes only url_vcs and url_notebook, ignores link
    Optional steps:
  • path is removed from pg_examples, infrastructure merged, examples CD step applied
  • link field removed from proto and marked reserved
  • backend, frontend deployed

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

  • Choose reviewer(s) and mention them in a comment (R: @username).
  • Mention the appropriate issue in your description (for example: addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment fixes #<ISSUE NUMBER> instead.
  • Update CHANGES.md with noteworthy changes.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md

GitHub Actions Tests Status (on master branch)

Build python source distribution and wheels
Python tests
Java tests
Go tests

See CI.md for more information about GitHub Actions CI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

below is a long-lasting issue with updating big number of entities in a transaction
really complicates testing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

need to keep path, to not break old backend

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there an issue tracking its removal? If not, could we add one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

too much work for this PR(

Copy link
Contributor

Choose a reason for hiding this comment

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

Same question as above - could we add an issue for this (or drop a link in this comment if one already exists)?

@eantyshev eantyshev marked this pull request as ready for review November 29, 2022 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.

Please change the order of words to 'VCS URL' here and everywhere else. Also 'notebook URL'.

This is a better English, and also Robert Martin suggests the most important words at the end, and 'URL' is definitive among the two.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMHO these concerns are less important than
the fact that these fields are logically related and are better be alphabetically adjacent

Copy link
Contributor Author

@eantyshev eantyshev Nov 30, 2022

Choose a reason for hiding this comment

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

there's no place we use could use it

Comment on lines 53 to 55
Copy link
Contributor

Choose a reason for hiding this comment

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

😱

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the refactoring is needed, but it would be too much for this change

Copy link
Contributor

Choose a reason for hiding this comment

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

A good opportunity to add trailing commas in this and other affected files.

Comment on lines +277 to +303
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you check the Python's habits for acronyms? In Dart and many other languages the habit is to capitalize only the first letter. For instance the class for URI is named Uri in Dart. I guess urlVcs would better in this case.

And also since we change to 'VCS URL' order this becomes vcsUrl and notebookUrl.

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 casing is already used in other Datastore fields in Playground

@eantyshev eantyshev changed the title [Playground] parse notebook_url example tag [Playground] parse url_notebook example tag Nov 30, 2022
@eantyshev eantyshev force-pushed the playground_notebook_url_infra branch from 8d975ee to 77ad06f Compare December 13, 2022 10:35
@github-actions
Copy link
Contributor

Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment assign set of reviewers

@eantyshev eantyshev force-pushed the playground_notebook_url_infra branch from 77ad06f to 28ac4a1 Compare December 13, 2022 11:59
Copy link
Contributor

@damccorm damccorm left a comment

Choose a reason for hiding this comment

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

LGTM, just had some minor nits

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there an issue tracking its removal? If not, could we add one?

Copy link
Contributor

Choose a reason for hiding this comment

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

Same question as above - could we add an issue for this (or drop a link in this comment if one already exists)?

@eantyshev
Copy link
Contributor Author

tasks to address the comments:
#24654
#24645

@eantyshev eantyshev requested a review from damccorm December 14, 2022 11:54
Copy link
Contributor

@damccorm damccorm left a comment

Choose a reason for hiding this comment

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

Thanks!

@damccorm damccorm merged commit ef5351a into apache:master Dec 14, 2022
@damccorm damccorm added the build label Dec 14, 2022
lostluck pushed a commit to lostluck/beam that referenced this pull request Dec 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Task]: Backend support for 'notebook' field in examples

3 participants