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

Sigma api e2e #1730

Merged
merged 31 commits into from
Apr 18, 2021
Merged

Sigma api e2e #1730

merged 31 commits into from
Apr 18, 2021

Conversation

jaegeral
Copy link
Collaborator

@jaegeral jaegeral commented Apr 1, 2021

Sigma 2e2 stuff, some other fixed in the api client and a Sigma Notebook

@google-cla google-cla bot added the cla: yes label Apr 1, 2021
@jaegeral jaegeral marked this pull request as ready for review April 1, 2021 13:05
@jaegeral jaegeral requested a review from kiddinn April 1, 2021 13:05
@jaegeral jaegeral added API client Code health Sigma Issued related to our Sigma integration labels Apr 1, 2021
Copy link
Contributor

@kiddinn kiddinn left a comment

Choose a reason for hiding this comment

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

mostly comments on the notebook

notebooks/Sigma_test_Notebook.ipynb Outdated Show resolved Hide resolved
notebooks/Sigma_test_Notebook.ipynb Outdated Show resolved Hide resolved
notebooks/Sigma_test_Notebook.ipynb Outdated Show resolved Hide resolved
notebooks/Sigma_test_Notebook.ipynb Outdated Show resolved Hide resolved
notebooks/Sigma_test_Notebook.ipynb Outdated Show resolved Hide resolved
notebooks/Sigma_test_Notebook.ipynb Outdated Show resolved Hide resolved
notebooks/Sigma_test_Notebook.ipynb Outdated Show resolved Hide resolved
@kiddinn
Copy link
Contributor

kiddinn commented Apr 7, 2021

I'm also missing some more text in the notebook, some further explanations on what is happening, why, what you are trying to achieve, etc, etc, etc.

@jaegeral
Copy link
Collaborator Author

jaegeral commented Apr 8, 2021

I'm also missing some more text in the notebook, some further explanations on what is happening, why, what you are trying to achieve, etc, etc, etc.

Good point, added a littel more text of what is going on, hope that helps.

@jaegeral jaegeral requested a review from kiddinn April 8, 2021 08:53
@jaegeral
Copy link
Collaborator Author

jaegeral commented Apr 8, 2021

Not sure why the tests are currently only queued I tried to restart them, but the changes since last run only affected the notebook.

@kiddinn
Copy link
Contributor

kiddinn commented Apr 8, 2021

Not sure why the tests are currently only queued I tried to restart them, but the changes since last run only affected the notebook.

don't worry about that... there is a build-up queue in the GH actions for the google org, so that's why it's taking some time. I cancelled all your runs, since I'll need to re-sync with master.

I'll do that once the queue has caught up with all the currently running GH actions in the TS repo

Copy link
Contributor

@kiddinn kiddinn left a comment

Choose a reason for hiding this comment

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

few more comments

notebooks/Sigma_test_Notebook.ipynb Show resolved Hide resolved
notebooks/Sigma_test_Notebook.ipynb Outdated Show resolved Hide resolved
notebooks/Sigma_test_Notebook.ipynb Outdated Show resolved Hide resolved
notebooks/Sigma_test_Notebook.ipynb Outdated Show resolved Hide resolved
@jaegeral jaegeral requested a review from kiddinn April 12, 2021 12:26
@jaegeral
Copy link
Collaborator Author

ready for next round of review

Copy link
Contributor

@kiddinn kiddinn left a comment

Choose a reason for hiding this comment

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

ok, almost there, just small nits and then we are good to go

notebooks/Sigma_test_Notebook.ipynb Outdated Show resolved Hide resolved
notebooks/Sigma_test_Notebook.ipynb Outdated Show resolved Hide resolved
notebooks/Sigma_test_Notebook.ipynb Outdated Show resolved Hide resolved
notebooks/Sigma_test_Notebook.ipynb Outdated Show resolved Hide resolved
@jaegeral jaegeral requested a review from kiddinn April 17, 2021 15:23
@jaegeral
Copy link
Collaborator Author

ok, almost there, just small nits and then we are good to go

The nits have been knitted

Copy link
Contributor

@kiddinn kiddinn left a comment

Choose a reason for hiding this comment

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

LGTM

@kiddinn kiddinn merged commit 4716157 into google:master Apr 18, 2021
@jaegeral jaegeral deleted the sigma_api_e2e branch October 8, 2021 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API client cla: yes Sigma Issued related to our Sigma integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants