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

🌱 Remove tilt_modules #534

Merged
merged 1 commit into from
Feb 25, 2022
Merged

🌱 Remove tilt_modules #534

merged 1 commit into from
Feb 25, 2022

Conversation

lentzi90
Copy link
Member

@lentzi90 lentzi90 commented Feb 18, 2022 •
edited
Loading

What this PR does / why we need it:

Tilt modules are automatically downloaded and does not need to be
included in source control. The latest release (v0.25) has also changed
to use the XDG data dir for tilt_modules instead of keeping them here
next to the Tiltfile.

See this comment: tilt-dev/tilt#5336 (comment)

It also seems like the cert-manager version setting was ignored in the Tiltfile. This is fixed by this PR.

@metal3-io-bot metal3-io-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 18, 2022
@lentzi90 lentzi90 force-pushed the lentzi90/remove-tilt-modules branch 2 times, most recently from e891e7d to e21f24c Compare February 21, 2022 09:23
@metal3-io-bot metal3-io-bot removed the needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. label Feb 21, 2022
@lentzi90
Remove tilt_modules
bf842d7 
Tilt modules are automatically downloaded and does not need to be
included in source control. The latest release (v0.25) has also changed
to use the XDG data dir for tilt_modules instead of keeping them here
next to the Tiltfile.

Also fix ignored cert-manager version when running with tilt.
@lentzi90
Copy link
Member Author

/test-v1b1-ubuntu-integration

@lentzi90
Copy link
Member Author

I have confirmed locally that tilt still works and also that the cert-manager version is now correctly set.

fmuyassarov
fmuyassarov reviewed Feb 24, 2022
View reviewed changes
Copy link
Member

@fmuyassarov fmuyassarov 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
/approve

@metal3-io-bot metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 24, 2022
furkatgofurov7
furkatgofurov7 reviewed Feb 24, 2022
View reviewed changes
Copy link
Member

@furkatgofurov7 furkatgofurov7 left a comment

Choose a reason for hiding this comment

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

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Feb 24, 2022
@furkatgofurov7
Copy link
Member

/hold

@metal3-io-bot metal3-io-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 24, 2022
@furkatgofurov7
Copy link
Member

Running this locally noticed some unwanted changes, holding

@metal3-io-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fmuyassarov, Rozzii

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@furkatgofurov7
Copy link
Member

furkatgofurov7 commented Feb 24, 2022 •
edited
Loading

While running Tilt from this branch locally (make tilt-settings, make tilt-up), I have noticed that the all files you deleted under tilt_modules folder are re-generated. The first impression I had from the PR description was we would not need those files at all but I do not like the fact that they will be still re-generated and will be there when running it locally.
I also checked other providers, and found out:

@lentzi90
Copy link
Member Author

@furkatgofurov7 yes Tilt will re-generate the tilt_modules automatically when you run it. Depending on what version you have they will be in the working directory (older versions) or in the XDG data directory (for latest Tilt).

What they did in CAPI was to get rid of the extensions and instead use their own way of installing cert-manager. We could do something similar but that is a bigger change. My point with this was just to not have the tilt_modules in git since they are autogenerated by Tilt.

@furkatgofurov7
Copy link
Member

/hold cancel

@metal3-io-bot metal3-io-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 25, 2022
@metal3-io-bot metal3-io-bot merged commit 255a4e6 into metal3-io:main Feb 25, 2022
@lentzi90 lentzi90 deleted the lentzi90/remove-tilt-modules branch February 25, 2022 08:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fmuyassarov fmuyassarov fmuyassarov left review comments

@furkatgofurov7 furkatgofurov7 furkatgofurov7 left review comments

@Rozzii Rozzii Rozzii approved these changes

Assignees
No one assigned
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@lentzi90 @furkatgofurov7 @metal3-io-bot @Rozzii @fmuyassarov