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

Refactoring events to use Config interface for init #5532

Merged
merged 2 commits into from Mar 11, 2021

Conversation

tejal29
Copy link
Member

@tejal29 tejal29 commented Mar 11, 2021

This is a refactor in prep of making code review changes suggested in #5491

Problem

  1. Right now event.InitializeState passes a number of attributes from runcontext.RunContext. for Add Cluster Internal Service Error code along with runcontext #5491, i want to pass more parameters from RunContext to event handler. This will make the signature more long.

Solution

  1. Add a Config interface similar to Introduce Config interfaces #4598 and add methods to access fields needed.

The other change in this PR is

  1. Add a helper method in testutil.event package to avoid circular dependencies and use it everywhere in tests to initialize test.

@codecov
Copy link

codecov bot commented Mar 11, 2021

Codecov Report

Merging #5532 (4200976) into master (b5b916d) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5532      +/-   ##
==========================================
+ Coverage   71.32%   71.34%   +0.02%     
==========================================
  Files         400      400              
  Lines       14829    14829              
==========================================
+ Hits        10577    10580       +3     
+ Misses       3475     3474       -1     
+ Partials      777      775       -2     
Impacted Files Coverage Δ
pkg/skaffold/event/event.go 90.78% <100.00%> (ø)
pkg/skaffold/runner/new.go 67.58% <100.00%> (ø)
pkg/skaffold/docker/image.go 79.53% <0.00%> (+1.39%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b5b916d...4200976. Read the comment docs.

Copy link
Collaborator

@gsquared94 gsquared94 left a comment

Choose a reason for hiding this comment

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

lgtm, weird coincidence I was gonna make the same PR today since I wanted to introduce new event properties also :)

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
Copy link
Collaborator

Choose a reason for hiding this comment

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

super nit: filename config.go instead of helper.go?

@tejal29 tejal29 merged commit f47c47b into GoogleContainerTools:master Mar 11, 2021
@tejal29 tejal29 deleted the refactor_event_init branch July 21, 2021 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants