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

fix parentspan #1579

Merged
merged 5 commits into from
May 1, 2022
Merged

fix parentspan #1579

merged 5 commits into from
May 1, 2022

Conversation

golanha
Copy link
Member

@golanha golanha commented Apr 25, 2022

This change is Reviewable

Copy link
Contributor

@yehiyam yehiyam left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1, 2 of 3 files at r2, all commit messages.
Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @golanha and @yehiyam)


core/api-server/tests/exec-pipelines.js line 19 at r2 (raw file):

            restPath = `${restUrl}/exec/pipelines`;

            tracer.startSpanOld = tracer.startSpan;

use sinon.spy


core/api-server/tests/exec-pipelines.js line 156 at r2 (raw file):

            };
            const response = await request(options);
            expect(rootSpan).to.eq('abcd');

don't add it here. Create a new test for this

@yehiyam yehiyam self-requested a review April 26, 2022 13:37
Copy link
Member Author

@golanha golanha left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @yehiyam)


core/api-server/tests/exec-pipelines.js line 19 at r2 (raw file):

Previously, yehiyam wrote…

use sinon.spy

Done.


core/api-server/tests/exec-pipelines.js line 156 at r2 (raw file):

Previously, yehiyam wrote…

don't add it here. Create a new test for this

Done.

Copy link
Contributor

@yehiyam yehiyam left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @golanha)

@golanha golanha enabled auto-merge (squash) May 1, 2022 12:23
@golanha golanha merged commit f609a51 into master May 1, 2022
@golanha golanha deleted the parentSpan branch May 1, 2022 12:27
hkube-ci pushed a commit that referenced this pull request May 1, 2022
* fix parentspan

* add unit test

* CR

Co-authored-by: yehiyam <yehiyam@users.noreply.github.com> .... bump version [skip ci]
hkube-ci pushed a commit that referenced this pull request May 1, 2022
* fix parentspan

* add unit test

* CR

Co-authored-by: yehiyam <yehiyam@users.noreply.github.com> .... bump version [skip ci]
@golanha golanha restored the parentSpan branch May 1, 2022 12:32
@golanha golanha deleted the parentSpan branch May 1, 2022 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants