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

use commit time of latest commit for BUILD date #16196

Merged
merged 1 commit into from
Nov 17, 2017

Conversation

durandom
Copy link
Member

@durandom durandom commented Oct 13, 2017

in case a BUILD file is missing we used the current time for
the build time - which can be really confusing

Not using Time.zone on purpose - because we want UTC

This did cost us quite some time in debugging https://bugzilla.redhat.com/show_bug.cgi?id=1491768

The posted build date master.20170914082537_bf64a63 suggested that #15590 which would fix the issue is not on the appliance - but the sha bf64a63 appended to the build version was commited on 2017-08-29 09:54:25

@miq-bot assign @Fryguy

@durandom
Copy link
Member Author

@simaishi can you say when a BUILD file would be missing? I used a .box (vagrant) upstream build and it wasnt there....

@simaishi
Copy link
Contributor

Upstream doesn't have BUILD file, that's downstream only. (I'm not sure why... and I'm thinking we should probably have it upstream too)

@chessbyte chessbyte requested a review from simaishi October 13, 2017 13:54
@Fryguy
Copy link
Member

Fryguy commented Oct 13, 2017

I don't agree with the date of the last commit. The reason is, you can bundle update on the same commit, get different gems, and the build is still different. Also, the build includes manageiq-aplliance, which is a totally different repo that also has its own commit. IMO, as @simaishi says, we should create a BUILD file with the date at the moment of building.

@durandom
Copy link
Member Author

The reason is, you can bundle update on the same commit, get different gems, and the build is still different

@Fryguy great catch and totally agree with that. But imho anythings better than Time.now, cause thats really misleading. Maybe we replace it by unknown?

@simaishi would you add that BUILD file to upstream builds? Or point me to a place where I can do it?

@simaishi
Copy link
Contributor

Agree Time.now is really confusing... But once we start creating BUILD file for appliances, what we put here becomes less important, as you'll see the output of this line only on dev machines. So whatever works for developers 😄

I'm thinking BUILD file can be added in source_setup.ks.erb. This is where git clone happens.

@durandom
Copy link
Member Author

@Fryguy please have a look again. I just remove Time.now and replaced it with unknown

in case a BUILD file is missing we used the current time for
the build time - which can be really confusing
@durandom
Copy link
Member Author

ManageIQ/manageiq-appliance-build#236 adds a BUILD file to upstream appliances

@miq-bot
Copy link
Member

miq-bot commented Oct 23, 2017

Checked commit durandom@ff44f64 with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks fine. 👍

@durandom
Copy link
Member Author

@Fryguy @carbonin you want to merge this?

Copy link
Member

@carbonin carbonin left a comment

Choose a reason for hiding this comment

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

So now the only time we won't have a BUILD file is development, and maybe in containers, correct?

I'm okay with this, if nothing else printing the current time in the build field is misleading at best so I can't see how this isn't an improvement.

@carbonin carbonin self-assigned this Nov 17, 2017
@carbonin carbonin added this to the Sprint 74 Ending Nov 27, 2017 milestone Nov 17, 2017
@carbonin carbonin merged commit a014294 into ManageIQ:master Nov 17, 2017
@durandom durandom deleted the change_build_date branch November 17, 2017 22:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants