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

Enable setting SOURCE_DATA_EPOCH #1032

Merged
merged 5 commits into from
Sep 28, 2022
Merged

Enable setting SOURCE_DATA_EPOCH #1032

merged 5 commits into from
Sep 28, 2022

Conversation

yaelharel
Copy link
Contributor

This environment variable is the timestamp for created time in app image config

Fixes #1009

Signed-off-by: Yael Harel yharel@vmware.com

Enable setting SOURCE_DATA_EPOCH
53cb3fe 
This environment variable is the timestamp for created time in app image config

Fixes #1009

Signed-off-by: Yael Harel <yharel@vmware.com>
@yaelharel @tomkennedy513
update the envs function to support multiple EnvVars.
852d895 
Co-authored-by: Tom Kennedy <10608054+tomkennedy513@users.noreply.github.com>
@tomkennedy513
Copy link
Collaborator

tomkennedy513 commented Sep 13, 2022 •
edited
Loading

Do we want this feature to be also configurable at the controller level or at just the image level

setEpochTime in image_builds_test
16f1667 
Signed-off-by: Yael Harel <yharel@vmware.com>
@sambhav
Copy link
Contributor

sambhav commented Sep 13, 2022

Should we try and match the pack behavior for this? i.e. allowing users to provide a iso timestamp or the word now?

@yaelharel
Copy link
Contributor Author

Hi @samj1912, thanks for your comment.
We thought about adding this option in kpack-cli.
What do you think?

@matthewmcnew
Copy link
Collaborator

I think kpack needs to support the same configuation options as pack and this should be a non-boolean field that accepts similar inputs to pack's --creation-time flag.

@matthewmcnew
Copy link
Collaborator

We thought about adding this option in buildpacks-community/kpack-cli#267.

@yaelharel A --creation-time flag on the kp cli would require exposing that functionality on the image/build spec as well, right?

@tylerphelan
Copy link
Contributor

@samj1912 @matthewmcnew @yaelharel

does using a timestamp make sense as an input to the image resource? or only the build resource? I supposed it doesn't hurt..

@tylerphelan
Copy link
Contributor

@yaelharel let's support both unix timestamp and now because it's kinda nice to just pass values to the lifecycle

Support both unix timestamp and "now" as a creation time
2a46eaf 
Signed-off-by: Yael Harel <yharel@vmware.com>
matthewmcnew
matthewmcnew reviewed Sep 16, 2022
View reviewed changes
pkg/apis/build/v1alpha2/image_validation.go
@@ -164,6 +165,14 @@ func (ib *ImageBuild) Validate(ctx context.Context) *apis.FieldError {
}
}

if ib.CreationTime != "" && ib.CreationTime != "now" {
// check that the timestamp in CreationTime is in valid format
_, err := strconv.ParseInt(ib.CreationTime, 10, 64)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice!

tomkennedy513
tomkennedy513 approved these changes Sep 16, 2022
View reviewed changes
Copy link
Collaborator

@tomkennedy513 tomkennedy513 left a comment

Choose a reason for hiding this comment

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

This is great!

@tylerphelan
Copy link
Contributor

tylerphelan commented Sep 16, 2022 •
edited
Loading

let's hold off on merging this until kpack 0.7.1 is released: https://github.com/pivotal/kpack/milestone/6

@codecov-commenter
Copy link

Codecov Report

Merging #1032 (104da8f) into main (f4c8582) will decrease coverage by 0.38%.
The diff coverage is 65.51%.

@@            Coverage Diff             @@
##             main    #1032      +/-   ##
==========================================
- Coverage   70.44%   70.05%   -0.39%     
==========================================
  Files         122      122              
  Lines        5742     5768      +26     
==========================================
- Hits         4045     4041       -4     
- Misses       1321     1340      +19     
- Partials      376      387      +11
Impacted Files Coverage Δ
pkg/apis/build/v1alpha2/build_types.go 100.00% <ø> (ø)
pkg/apis/build/v1alpha2/image_types.go 50.00% <ø> (ø)
pkg/apis/build/v1alpha2/build_pod.go 97.38% <58.33%> (-1.28%) ⬇️
pkg/apis/build/v1alpha2/image_builds.go 58.42% <100.00%> (-11.07%) ⬇️
pkg/apis/build/v1alpha2/image_validation.go 98.27% <100.00%> (+0.06%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@yaelharel yaelharel merged commit 367ce42 into main Sep 28, 2022
@yaelharel yaelharel deleted the creationTime branch September 28, 2022 14:12
@chenbh chenbh mentioned this pull request Oct 31, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tylerphelan tylerphelan tylerphelan left review comments

@tomkennedy513 tomkennedy513 tomkennedy513 approved these changes

@matthewmcnew matthewmcnew matthewmcnew approved these changes

@ncarlson ncarlson ncarlson approved these changes

@pviraj59 pviraj59 Awaiting requested review from pviraj59

@chenbh chenbh Awaiting requested review from chenbh

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Enable configuring the creation time
7 participants
@yaelharel @tomkennedy513 @sambhav @matthewmcnew @tylerphelan @codecov-commenter @ncarlson