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

Design for generic artifact fetching #652

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

kosciCZ
Copy link
Contributor

@kosciCZ kosciCZ commented Sep 11, 2024

Maintainers will complete the following section

  • Commit messages are descriptive enough
  • Code coverage from testing does not decrease and new code is covered
  • Docs updated (if applicable)
  • Docs links in the code are still valid (if docs were updated)

Note: if the contribution is external (not from an organization member), the CI
pipeline will not run automatically. After verifying that the CI is safe to run:

@kosciCZ
Copy link
Contributor Author

kosciCZ commented Sep 11, 2024

@brunoapimentel PTAL

@kosciCZ kosciCZ force-pushed the generic-artifact-fetching-design branch from 1ec4342 to ae7a50c Compare September 11, 2024 13:36
@eskultety eskultety changed the title [ISV-5094] Design for generic artifact fetching Design for generic artifact fetching Sep 11, 2024
docs/design/generic.md Outdated Show resolved Hide resolved
docs/design/generic.md Outdated Show resolved Hide resolved
docs/design/generic.md Outdated Show resolved Hide resolved
docs/design/generic.md Outdated Show resolved Hide resolved
@kosciCZ kosciCZ force-pushed the generic-artifact-fetching-design branch 2 times, most recently from 65628a0 to b6fd32b Compare September 17, 2024 08:30
docs/design/generic.md Outdated Show resolved Hide resolved
docs/design/generic.md Outdated Show resolved Hide resolved
Copy link
Contributor

@brunoapimentel brunoapimentel left a comment

Choose a reason for hiding this comment

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

Leaving some additional comments just to clarify some points, but I believe we have a solid direction to start the implementation.

docs/design/generic.md Outdated Show resolved Hide resolved
docs/design/generic.md Outdated Show resolved Hide resolved
Copy link
Member

@eskultety eskultety left a comment

Choose a reason for hiding this comment

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

Nothing major just quick nitpick fixes. In general I am in agreement. With the first ideas of contributing guidelines having materialized recently we can merge this OR keep it open until the implementation work is done, update sections as necessary should some further technical considerations be made during development, etc. I'd be fine either way. That said, if you wish to merge this right away then please squash all commits into a single one, no fixups please.

docs/design/generic.md Outdated Show resolved Hide resolved
docs/design/generic.md Outdated Show resolved Hide resolved
docs/design/generic.md Outdated Show resolved Hide resolved
docs/design/generic.md Outdated Show resolved Hide resolved
docs/design/generic.md Outdated Show resolved Hide resolved
Copy link
Contributor

@brunoapimentel brunoapimentel left a comment

Choose a reason for hiding this comment

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

Giving my ack here since I'm also in agreement with the design, but I'd suggest not to merge this (as Erik commented before), and turn it in an ADR when the feature is production-ready.

@lkolacek
Copy link
Contributor

lkolacek commented Oct 1, 2024

Giving my ack here since I'm also in agreement with the design, but I'd suggest not to merge this (as Erik commented before), and turn it in an ADR when the feature is production-ready.

+1

Copy link
Contributor

@lkolacek lkolacek left a comment

Choose a reason for hiding this comment

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

LGTM +1, thank you guys!

Signed-off-by: Jan Koscielniak <[email protected]>
Changelog:
- lockfile has two ways of specifying the url
- add a note about ExternalReferences in the SBOM
- explicitely reference EC rule used to validate download url
- formatting

Signed-off-by: Jan Koscielniak <[email protected]>
Signed-off-by: Jan Koscielniak <[email protected]>
Signed-off-by: Jan Koscielniak <[email protected]>
@kosciCZ kosciCZ force-pushed the generic-artifact-fetching-design branch from efcd917 to b1020cf Compare October 1, 2024 11:06
@kosciCZ
Copy link
Contributor Author

kosciCZ commented Oct 1, 2024

Thanks for all your reviews, I'll keep this open and will then edit it into ADR when we're done with the implementation.

@kosciCZ kosciCZ mentioned this pull request Oct 10, 2024
4 tasks
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.

4 participants