Skip to content

extensions: Separate generation of extension db from docs in CI#15117

Merged
htuch merged 13 commits intoenvoyproxy:mainfrom
phlax:extensions-separate-db-generation
Mar 2, 2021
Merged

extensions: Separate generation of extension db from docs in CI#15117
htuch merged 13 commits intoenvoyproxy:mainfrom
phlax:extensions-separate-db-generation

Conversation

@phlax
Copy link
Copy Markdown
Member

@phlax phlax commented Feb 19, 2021

Signed-off-by: Ryan Northey ryan@synca.io

Commit Message: extensions: Separate generation of extension db from docs in CI
Additional Description:

As we will require the extension DB to be generated for docs and schema, it makes sense to separate this task so that those jobs can run in parallel

Im wondering if this job should be moved to a bazel build somewhere - but initially im just gonna get it working by splitting the script/job

UPDATE

rather than separating the ci job, i have moved the db generation into a bazel job - which will allow other tasks that also require this data to make use of it more easily/bazely

this also adds buildozer into the bazel env, so we might want to make use of that elsewhere and potentially remove buildozer from the build-image env

Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue] Touch #13229
[Optional Deprecated:]
[Optional API Considerations:]

@phlax phlax changed the title [WIP] extensions: Separate generate of extension db from docs in CI [WIP] extensions: Separate generation of extension db from docs in CI Feb 19, 2021
@phlax phlax marked this pull request as draft February 19, 2021 15:00
@phlax phlax force-pushed the extensions-separate-db-generation branch 3 times, most recently from 57f11dc to e41dd41 Compare February 19, 2021 17:10
@phlax
Copy link
Copy Markdown
Member Author

phlax commented Feb 19, 2021

im not 100% how best to separate this, or exactly how it will be required by #15100 - so this PR is currently experimental to figure these things out

@phlax phlax force-pushed the extensions-separate-db-generation branch from e41dd41 to 12485ed Compare February 26, 2021 06:04
@repokitteh-read-only
Copy link
Copy Markdown

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).

🐱

Caused by: #15117 was synchronize by phlax.

see: more, trace.

@repokitteh-read-only repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Feb 26, 2021
@phlax phlax force-pushed the extensions-separate-db-generation branch 9 times, most recently from 2e06e9f to dbe4373 Compare February 26, 2021 17:58
@phlax phlax changed the title [WIP] extensions: Separate generation of extension db from docs in CI extensions: Separate generation of extension db from docs in CI Feb 26, 2021
@phlax phlax marked this pull request as ready for review February 26, 2021 18:09
@phlax
Copy link
Copy Markdown
Member Author

phlax commented Feb 26, 2021

@htuch would you mind having a look at this one

im not sure if its done the best way - using /source/... feels a bit hacky

also, i had to change the visibility of the actual //source and include in the bazel data - not sure if i understand why

@phlax phlax force-pushed the extensions-separate-db-generation branch from b4a3d0c to 1f65be9 Compare February 28, 2021 10:40
Signed-off-by: Ryan Northey <ryan@synca.io>
@phlax phlax force-pushed the extensions-separate-db-generation branch from 1f65be9 to dbffcf8 Compare February 28, 2021 11:11
Signed-off-by: Ryan Northey <ryan@synca.io>
Signed-off-by: Ryan Northey <ryan@synca.io>
@htuch
Copy link
Copy Markdown
Member

htuch commented Feb 28, 2021

@phlax generally this is a really nice step. I don't mind hardcoding /source as the default for ENVOY_SOURCE, but maybe you want to use ENVOY_SRCDIR as done elsewhere in ci/. The visibility stuff would benefit from better understanding, since I rather keep the limitation in at least some CI checked build to avoid accidental dependencies across core/extensions happening.

@repokitteh-read-only
Copy link
Copy Markdown

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #15117 (comment) was created by @phlax.

see: more, trace.

@phlax
Copy link
Copy Markdown
Member Author

phlax commented Mar 1, 2021

/retest

@repokitteh-read-only
Copy link
Copy Markdown

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #15117 (comment) was created by @phlax.

see: more, trace.

@phlax
Copy link
Copy Markdown
Member Author

phlax commented Mar 1, 2021

im pretty sure the coverage test fail is unrelated - retesting doesnt seem to fix

"//test/extensions/...",
"//test/server",
"//test/server/config_validation",
"//tools/extensions/...",
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

maybe this should be more specific - not sure if the elipsis is necessary

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd make is as narrow as works.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

afaict this is as narrow as i can get it

Signed-off-by: Ryan Northey <ryan@synca.io>
Copy link
Copy Markdown
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

LGTM modulo nits.
/wait

if __name__ == '__main__':
output_path = sys.argv[1]
try:
output_path = (os.environ["EXTENSION_DB_PATH"]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

output_path = os.getenv('EXTENSION_DB_PATH', sys.argv[1])

Copy link
Copy Markdown
Member Author

@phlax phlax Mar 2, 2021

Choose a reason for hiding this comment

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

this would break if sys.argv[1] is not set (ie even when EXTENSION_DB_PATH was) - hence the if ... else

i think this code could be a bit prettier with different formatting but i think the logic is correct

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

i updated to output_path = os.getenv("EXTENSION_DB_PATH") or sys.argv[1] which is more concise and works as expected

"//test/extensions/...",
"//test/server",
"//test/server/config_validation",
"//tools/extensions/...",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd make is as narrow as works.

Signed-off-by: Ryan Northey <ryan@synca.io>
phlax added 4 commits March 2, 2021 04:37
Signed-off-by: Ryan Northey <ryan@synca.io>
Signed-off-by: Ryan Northey <ryan@synca.io>
Signed-off-by: Ryan Northey <ryan@synca.io>
Signed-off-by: Ryan Northey <ryan@synca.io>
@phlax phlax requested a review from htuch March 2, 2021 05:12
Signed-off-by: Ryan Northey <ryan@synca.io>
@phlax
Copy link
Copy Markdown
Member Author

phlax commented Mar 2, 2021

/retest

@repokitteh-read-only
Copy link
Copy Markdown

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #15117 (comment) was created by @phlax.

see: more, trace.

Copy link
Copy Markdown
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@htuch
Copy link
Copy Markdown
Member

htuch commented Mar 2, 2021

/lgtm deps

@repokitteh-read-only repokitteh-read-only bot removed the deps Approval required for changes to Envoy's external dependencies label Mar 2, 2021
@htuch htuch merged commit a03209d into envoyproxy:main Mar 2, 2021
@moderation
Copy link
Copy Markdown
Contributor

@phlax I'll be having a stern word with @htuch on his deps LGTM 😄 The version is hardcoded in strip_prefix and urls. Updated for 4.0.1 we should have

    com_github_bazelbuild_buildtools = dict(
        project_name = "Bazel build tools",
        project_desc = "Developer tools for working with Google's bazel buildtool.",
        project_url = "https://github.com/bazelbuild/buildtools",
        version = "4.0.1",
        sha256 = "0d3ca4ed434958dda241fb129f77bd5ef0ce246250feed2d5a5470c6f29a77fa",
        release_date = "2021-03-01",
        strip_prefix = "buildtools-{version}",
        urls = ["https://github.com/bazelbuild/buildtools/archive/{version}.tar.gz"],
        use_category = ["api"],
    ),

@phlax
Copy link
Copy Markdown
Member Author

phlax commented Mar 2, 2021

i can add a cleanup if you want

@htuch
Copy link
Copy Markdown
Member

htuch commented Mar 2, 2021

@moderation good point; can we add a check on this kind of hardcoding to the deps validation Python?

@moderation
Copy link
Copy Markdown
Contributor

A check in deps validation definitely makes sense. Will make an attempt.

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.

3 participants