Skip to content

libs: Add envoy.dependency.check[release-issues/v0.0.1]#143

Merged
kfaseela merged 1 commit intoenvoyproxy:mainfrom
phlax:envoy.dependency.check
Jan 29, 2022
Merged

libs: Add envoy.dependency.check[release-issues/v0.0.1]#143
kfaseela merged 1 commit intoenvoyproxy:mainfrom
phlax:envoy.dependency.check

Conversation

@phlax
Copy link
Copy Markdown
Member

@phlax phlax commented Dec 20, 2021

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

@netlify
Copy link
Copy Markdown

netlify bot commented Dec 20, 2021 •
edited
Loading

✔️ Deploy Preview for nifty-bassi-e26446 ready!

🔨 Explore the source changes: e67372f

🔍 Inspect the deploy log: https://app.netlify.com/sites/nifty-bassi-e26446/deploys/61f270436c51010007ac5442

😎 Browse the preview: https://deploy-preview-143--nifty-bassi-e26446.netlify.app

@phlax phlax marked this pull request as draft December 20, 2021 09:39
@phlax phlax mentioned this pull request Dec 20, 2021
Merged
@phlax phlax force-pushed the envoy.dependency.check branch from aa75cfa to 193cfac Compare December 21, 2021 15:09
@phlax phlax mentioned this pull request Dec 21, 2021
Merged
@phlax phlax force-pushed the envoy.dependency.check branch 5 times, most recently from 85f2281 to 5eb434c Compare December 24, 2021 10:47
@phlax phlax force-pushed the envoy.dependency.check branch 2 times, most recently from 25d91ef to 0230112 Compare December 29, 2021 17:45
phlax
phlax commented Dec 29, 2021
View reviewed changes
@phlax phlax mentioned this pull request Dec 29, 2021
Merged
@phlax phlax force-pushed the envoy.dependency.check branch 16 times, most recently from 1f6214e to e6cd3c9 Compare January 5, 2022 13:47
@phlax phlax force-pushed the envoy.dependency.check branch 3 times, most recently from 90422d4 to 5391df5 Compare January 17, 2022 13:31
@phlax phlax changed the title [WIP] libs: Add envoy.dependency.check libs: Add envoy.dependency.check Jan 17, 2022
@phlax phlax marked this pull request as ready for review January 17, 2022 13:39
@phlax phlax force-pushed the envoy.dependency.check branch 2 times, most recently from 8c88dfc to 407ecae Compare January 18, 2022 12:12
phlax
phlax commented Jan 18, 2022
View reviewed changes
phlax
phlax commented Jan 18, 2022
View reviewed changes
kfaseela
kfaseela reviewed Jan 19, 2022
View reviewed changes
envoy.dependency.check/envoy/dependency/check/abstract/dependency.py
@property
def organization(self) -> str:
"""Github organization name."""
return self.url_components[3]
Copy link
Copy Markdown
Collaborator

@kfaseela kfaseela Jan 19, 2022

Choose a reason for hiding this comment

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

should we have all these indices defined as constants so that it is easier to understand?

Copy link
Copy Markdown
Member Author

@phlax phlax Jan 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

this is one of the bits i least like about this implementation

what i would like to do is add a GithubURLParser that can tell you

  • the type of url
  • org/user/repo
  • additional info according to type

and that would raise appropriate errors

i havent found anything that does this already, but i havent looked that hard either

as this will require a bit of thought - for now i would prefer to keep the existing implementation (im just adding a bit of error handling to it)

if its a blocker tho, we can rethink the url parsing

one consideration here is that the URL is the SoA for the repo connect - this means that configs like here https://github.com/envoyproxy/envoy/pull/19473/files#diff-a2ca502474bb8ec5f092176f7ee6d2f11fc395ebb00e1717d6fbad9b16ee8fd8R481 wont work

(i will comment this shortly 8/)

tldr i think this needs quite a bit of a rethink - but this is close to what it does now and works to our current expectations, but i would prefer not to formalize it

@phlax phlax mentioned this pull request Jan 19, 2022
Open
3 tasks
@phlax phlax force-pushed the envoy.dependency.check branch from a93e32a to c271353 Compare January 19, 2022 13:16
@phlax
Copy link
Copy Markdown
Member Author

phlax commented Jan 19, 2022

force pushed to fix dco - apologies

@phlax phlax force-pushed the envoy.dependency.check branch 2 times, most recently from a1aa2d7 to f2db837 Compare January 19, 2022 22:30
@phlax
Copy link
Copy Markdown
Member Author

phlax commented Jan 19, 2022

reWIPping this until #186 lands

@phlax phlax marked this pull request as draft January 19, 2022 22:31
@phlax phlax changed the title libs: Add envoy.dependency.check [WIP] libs: Add envoy.dependency.check[dates/releases/issues] Jan 19, 2022
@phlax phlax force-pushed the envoy.dependency.check branch 2 times, most recently from cde017e to ddf46cb Compare January 20, 2022 17:24
@phlax phlax changed the title [WIP] libs: Add envoy.dependency.check[dates/releases/issues] [WIP] libs: Add envoy.dependency.check[releases/releaseissues] Jan 20, 2022
@phlax phlax changed the title [WIP] libs: Add envoy.dependency.check[releases/releaseissues] [WIP] libs: Add envoy.dependency.check[releases/release-issues] Jan 20, 2022
@phlax
Copy link
Copy Markdown
Member Author

phlax commented Jan 20, 2022

also waiting for #187

@phlax phlax force-pushed the envoy.dependency.check branch from ddf46cb to 9251224 Compare January 21, 2022 11:50
@phlax
libs: Add envoy.dependency.check[release-issues]
e67372f 
Signed-off-by: Ryan Northey <ryan@synca.io>
htuch
htuch reviewed Jan 28, 2022
View reviewed changes
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 one question.

envoy.dependency.check/envoy/dependency/check/abstract/issues.py
raise NotImplementedError

@async_property(cache=True)
async def missing_labels(self) -> Tuple[str, ...]:
Copy link
Copy Markdown
Member

@htuch htuch Jan 28, 2022

Choose a reason for hiding this comment

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

There is a lot of logic in this file that I think might also apply to other situations where we want to automagically create, dedupe and label issues. Would it make sense to factor this out?

Copy link
Copy Markdown
Member Author

@phlax phlax Jan 28, 2022

Choose a reason for hiding this comment

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

yep, that makes sense - i have made some progress on this already making it more generic so we can cve_issues

the bit im not entirely clear about is where to factor it out to - perhaps to aio.api.github if the logic is generic enough

can i do this as/when i add the cve issues as this interface will require a bit of rethinking then anyway

Copy link
Copy Markdown
Member

@htuch htuch Jan 28, 2022

Choose a reason for hiding this comment

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

Sure, just wanted to flag this as worth considering.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kfaseela kfaseela kfaseela approved these changes

@htuch htuch htuch approved these changes

Assignees

@kfaseela kfaseela

@htuch htuch

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@phlax @kfaseela @htuch