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

Switch to using tilt-support repo #307

Merged
merged 1 commit into from
Aug 2, 2023

Conversation

ncdc
Copy link
Member

@ncdc ncdc commented Aug 1, 2023

Description

Requires operator-framework/tilt-support#1

Reviewer Checklist

  • API Go Documentation
  • Tests: Unit Tests (and E2E Tests, if appropriate)
  • Comprehensive Commit Messages
  • Links to related GitHub Issue(s)

Signed-off-by: Andy Goldstein <andy.goldstein@redhat.com>
@ncdc ncdc requested a review from a team as a code owner August 1, 2023 18:29
@codecov
Copy link

codecov bot commented Aug 1, 2023

Codecov Report

Merging #307 (2532cff) into main (949c06a) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #307   +/-   ##
=======================================
  Coverage   80.69%   80.69%           
=======================================
  Files          18       18           
  Lines         803      803           
=======================================
  Hits          648      648           
  Misses        112      112           
  Partials       43       43           
Flag Coverage Δ
e2e 55.79% <ø> (ø)
unit 77.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

# This loads a helper function that isn't part of core Tilt that simplifies restarting the process in the container
# when files changes.
load('ext://restart_process', 'docker_build_with_restart')
if not os.path.exists('../tilt-support'):
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm very leery about going outside the repo.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is for local development and iteration. It's pretty standard.

Copy link
Member Author

Choose a reason for hiding this comment

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

This doesn't clone the tilt-support repo. That's something you have to do manually. It's definitely not forcing arbitrary code from the internet on you 😄

An alternative is to consider tilt-support a Tilt extension:

v1alpha1.extension_repo(name='tilt-support', url='https://github.com/operator-framework/tilt-support')
v1alpha1.extension(name='tilt-support', repo_name='tilt-support', repo_path='.')
load('ext://tilt-support', 'deploy_repo')

Both would load the same code

Copy link
Contributor

Choose a reason for hiding this comment

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

I just said I'm "leery". I haven't seen many (if any?) projects that look outside their repository unless explicitly configured to do so by the user (e.g. --with-software=/path/to/software). Then again, I'm also leery about pulling random software versions from the 'net (given my corporate security background).
That said, if it's just looking for something, then it's not so bad.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, cool. Yeah this is by far the easiest way.

Copy link
Contributor

@perdasilva perdasilva left a comment

Choose a reason for hiding this comment

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

lgtm

@perdasilva perdasilva added this pull request to the merge queue Aug 2, 2023
Merged via the queue into operator-framework:main with commit bacf52b Aug 2, 2023
9 checks passed
@ncdc ncdc deleted the use-tilt-support-repo branch August 4, 2023 16:56
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.

None yet

4 participants