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.

Sign up for GitHub

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

Jump to bottom

[FEGA usecase] Merge ingest & verify from sda-pipeline into sda #304

Merged
merged 9 commits into from
Oct 11, 2023
Merged

[FEGA usecase] Merge ingest & verify from sda-pipeline into sda #304

merged 9 commits into from
Oct 11, 2023

Conversation

jbygdell
Copy link
Collaborator

@jbygdell jbygdell commented Sep 15, 2023 •
edited
Loading

  • Merges ingest and verify into sda
  • Expands test suite to ingest and verify an uploaded file.

@jbygdell jbygdell self-assigned this Sep 15, 2023
@jbygdell jbygdell force-pushed the merge_sda_pipeline_2 branch from 7334d33 to 2bf326c Compare September 15, 2023 15:18
@jbygdell jbygdell changed the title Merge ingest & verify from sda-pipeline into sda [FEGA usecase] Merge ingest & verify from sda-pipeline into sda Sep 17, 2023
@jbygdell jbygdell changed the base branch from main to merge_sda_pipeline_1 September 17, 2023 14:44
@jbygdell jbygdell force-pushed the merge_sda_pipeline_1 branch from 6efb28c to 0619bf2 Compare September 18, 2023 16:25
@jbygdell jbygdell force-pushed the merge_sda_pipeline_2 branch 2 times, most recently from 9c95ca1 to 3df9ec6 Compare September 18, 2023 16:43
@jbygdell jbygdell force-pushed the merge_sda_pipeline_1 branch 2 times, most recently from 2b87991 to 2d56356 Compare September 22, 2023 12:37
@jbygdell jbygdell force-pushed the merge_sda_pipeline_2 branch from 3df9ec6 to ef635a9 Compare September 22, 2023 12:40
Base automatically changed from merge_sda_pipeline_1 to main September 25, 2023 16:55
@jbygdell jbygdell force-pushed the merge_sda_pipeline_2 branch from 933a973 to 7eb0211 Compare September 26, 2023 07:09
@jbygdell jbygdell marked this pull request as ready for review September 26, 2023 07:20
@jbygdell jbygdell requested a review from a team September 26, 2023 07:20
@jbygdell jbygdell force-pushed the merge_sda_pipeline_2 branch from 7eb0211 to 49bf207 Compare September 28, 2023 18:21
@jbygdell jbygdell requested a review from kjellp September 29, 2023 06:47
pontus
pontus reviewed Sep 29, 2023
View reviewed changes
sda/cmd/ingest/ingest.md Outdated Show resolved Hide resolved
@jbygdell jbygdell force-pushed the merge_sda_pipeline_2 branch from 1f544d0 to 219413f Compare September 30, 2023 14:58
pontus
pontus previously approved these changes Oct 1, 2023
View reviewed changes
@pontus
Copy link
Contributor

pontus commented Oct 1, 2023

Saw some things (e.g. a sequence of 1. in documentation) that are not new and I'll try to remember to create issues for once merged.

aaperis
aaperis reviewed Oct 2, 2023
View reviewed changes
sda/cmd/ingest/ingest.go Outdated Show resolved Hide resolved
sda/cmd/ingest/ingest.go Outdated Show resolved Hide resolved
sda/cmd/ingest/ingest.go Outdated Show resolved Hide resolved
sda/cmd/ingest/ingest.go Outdated Show resolved Hide resolved
sda/cmd/ingest/ingest.go Outdated Show resolved Hide resolved
sda/cmd/verify/verify.go Outdated Show resolved Hide resolved
sda/cmd/verify/verify.go Outdated Show resolved Hide resolved
sda/cmd/verify/verify.go Outdated Show resolved Hide resolved
sda/cmd/verify/verify.go Outdated Show resolved Hide resolved
sda/cmd/verify/verify.go Outdated Show resolved Hide resolved
@jbygdell jbygdell force-pushed the merge_sda_pipeline_2 branch from d48699f to 0dbee1b Compare October 2, 2023 10:57
pontus
pontus previously approved these changes Oct 2, 2023
View reviewed changes
aaperis
aaperis previously approved these changes Oct 2, 2023
View reviewed changes
Copy link
Contributor

@aaperis aaperis left a comment

Choose a reason for hiding this comment

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

looks good

aaperis
aaperis reviewed Oct 3, 2023
View reviewed changes
Copy link
Contributor

@aaperis aaperis left a comment

Choose a reason for hiding this comment

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

minor typo in the logs

sda/cmd/ingest/ingest.go Outdated Show resolved Hide resolved
sda/cmd/ingest/ingest.go Outdated Show resolved Hide resolved
sda/cmd/ingest/ingest.go Outdated Show resolved Hide resolved
sda/cmd/ingest/ingest.go Outdated Show resolved Hide resolved
sda/cmd/verify/verify.go Outdated Show resolved Hide resolved
sda/cmd/verify/verify.go Outdated Show resolved Hide resolved
sda/cmd/verify/verify.go Outdated Show resolved Hide resolved
sda/cmd/verify/verify.go Outdated Show resolved Hide resolved
@jbygdell jbygdell dismissed stale reviews from aaperis and pontus via 8c0f632 October 3, 2023 14:54
@jbygdell jbygdell requested a review from a team October 4, 2023 06:41
@jbygdell jbygdell requested a review from a team October 5, 2023 06:34
@pontus
Copy link
Contributor

pontus commented Oct 5, 2023

Upon revisiting, I see managed to miss seeing all these log changes(!), and while they don't directly break things, they definitely seem to make the product worse to use to me.

While I don't think we had complete coverage before, it was fairly good - given e.g. a problematic file name you could easily find a related correlation id and from there filter out the entire flow, including errors.

With these changes, that's no longer possible because important information such as errors are no longer logged with these details, so troubleshooting will involve extra work with finding things logged close to particular lines we've received through filtering.

@kjellp
Copy link
Contributor

kjellp commented Oct 9, 2023 •
edited
Loading

I'm trying to locate the output of the intgeration/tests/sda/*.sh scripts (10_ and 20_ ) from the run github actions, but cannot see them anywhere? Thought I'd find them under sda or charts jobs, but was not able to locate them?

Not able to run them locally that easy...

Did run the dev_utils/run_integration_tests_no_tls.sh to see that ingest and verify did ok there (had to make one small tweak, correlation_id must be set to use a uuid instead of '1' + comment out expextation of inbox file being removed early in the pipeline).

@kjellp
Copy link
Contributor

kjellp commented Oct 9, 2023

Found them: under "Functionality tests" / functionality.yml, 4 jobs are defined to be triggered, including the sda-integration tests, but they are all marked as "was skipped" ?

kjellp
kjellp approved these changes Oct 10, 2023
View reviewed changes
Copy link
Contributor

@kjellp kjellp left a comment

Choose a reason for hiding this comment

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

LGTM, ran the ".github/integration/sda-s3-integration.yml run integration_test" locally as well and it completed successfully.

@jbygdell jbygdell merged commit a7310ee into main Oct 11, 2023
29 checks passed
@jbygdell jbygdell deleted the merge_sda_pipeline_2 branch October 11, 2023 06:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aaperis aaperis aaperis approved these changes

@kjellp kjellp kjellp approved these changes

@pontus pontus pontus left review comments

Assignees

@jbygdell jbygdell

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jbygdell @pontus @kjellp @aaperis