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

[CI] extract profile settings and build docker based on its values #13782

Merged
merged 7 commits into from
Oct 24, 2023
Merged

[CI] extract profile settings and build docker based on its values #13782

merged 7 commits into from
Oct 24, 2023

Conversation

dkijania
Copy link
Member

Explain your changes:
Exposed profile as parameter rather than hardcoded value in infra. As a consequence we will have set of dockers for each profile we define and would like to use

Explain how you tested your changes:

Run CI

Checklist:

  • Dependency versions are unchanged
    • Notify Velocity team if dependencies must change in CI
  • Modified the current draft of release notes with details on what is completed or incomplete within this project
  • Document code purpose, how to use it
    • Mention expected invariants, implicit constraints
  • Tests were added for the new behavior
    • Document test purpose, significance of failures
    • Test names should reflect their purpose
  • All tests pass (CI will check this if you didn't)
  • Serialized types are in stable-versioned modules
  • Does this close issues? List them
  • Closes #0000

@dkijania dkijania requested a review from psteckler July 28, 2023 17:54
@dkijania
Copy link
Member Author

!ci-build-me

@dkijania dkijania changed the title extract profile settings and build docker based on its values [CI] extract profile settings and build docker based on its values Aug 15, 2023
@dkijania dkijania force-pushed the dkijania/mina-profile-docker branch from 4b6a0d7 to 8ea7959 Compare August 16, 2023 08:25
@dkijania
Copy link
Member Author

!ci-build-me

@dkijania dkijania marked this pull request as ready for review August 17, 2023 08:19
@dkijania dkijania requested review from a team as code owners August 17, 2023 08:19
@dkijania
Copy link
Member Author

!ci-build-me

1 similar comment
@dkijania
Copy link
Member Author

!ci-build-me

@dkijania dkijania removed the request for review from psteckler September 21, 2023 20:48
@dkijania
Copy link
Member Author

!ci-build-me

1 similar comment
@dkijania
Copy link
Member Author

!ci-build-me

stevenplatt
stevenplatt suggested changes Sep 22, 2023
View reviewed changes
Copy link
Contributor

@stevenplatt stevenplatt left a comment

Choose a reason for hiding this comment

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

The mechanics of this PR are much needed, but I am unsure how to reconcile the changes to deb and docker names, with how these packages are being used publicly.

I double-checked the build steps running in compatible, to see if we are actively using the dune profile, but these CI jobs also use devnet flags. The functionality appears to be only a placeholder (at least within CI). Devnet+Berkeley may cause confusion internally as well, since devnet internally refers to the devnet testnet (which is a deployment based on compatible). Devnet = compatible and Develop = berkeley.

Would it provide a similar result to add an isolated build step for the lightnet container, while not modifying the existing deb and docker builds?

buildkite/src/Constants/Profiles.dhall Outdated
@@ -0,0 +1,25 @@
let Prelude = ../External/Prelude.dhall

let ProfileName = < Devnet | Lightnet | Hardfork >
Copy link
Contributor

Choose a reason for hiding this comment

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

Hardfork profile, nice!

buildkite/src/Jobs/Test/TestnetIntegrationTests.dhall Outdated
]
let dependsOnJs = [
{ name = "TestnetIntegrationTests", key = "build-test-executive" },
{ name = "TestnetIntegrationTests", key = "build-js-tests" },
{ name = "MinaArtifactBullseye", key = "daemon-berkeley-bullseye-docker-image" },
{ name = "MinaArtifactBullseye", key = "archive-bullseye-docker-image" }
{ name = "MinaArtifactBullseyeLightnet", key = "daemon-berkeley-bullseye-devnet-docker-image" },
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we able to use ${deb_profile}, ${DUNE_PROFILE}, or other var name in place of hardcoding devnet for docker container names?

Copy link
Member Author

Choose a reason for hiding this comment

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

I will remove devnet segment in key name

scripts/rebuild-deb.sh Outdated

copy_common_daemon_configs berkeley testnet 'seed-lists/berkeley_seeds.txt'

build_deb mina-berkeley
build_deb "mina-berkeley-${DUNE_PROFILE}"
Copy link
Contributor

Choose a reason for hiding this comment

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

For additional consideration, this may have impact for community users who has automations hardcoded to specific release channels. For example, it is possible that Coinbase builds their own containers, for the purpose of including additional tooling/monitoring. In this scenario, their automations may be coded to apt install mina-berkeley.deb or similar. Externally, I am unsure of how to reconcile this. Even though it can be included in release notes, large companies often will not follow every release, or catch that the deb name has changed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right! I will add suffix only if the profile is different than devnet to assure compatibility

@dkijania dkijania force-pushed the dkijania/mina-profile-docker branch from 4ebd90b to 10f63d5 Compare September 23, 2023 11:09
@dkijania
Copy link
Member Author

!ci-build-me

10 similar comments
@dkijania
Copy link
Member Author

!ci-build-me

@dkijania
Copy link
Member Author

!ci-build-me

@dkijania
Copy link
Member Author

!ci-build-me

@dkijania
Copy link
Member Author

!ci-build-me

@dkijania
Copy link
Member Author

!ci-build-me

@dkijania
Copy link
Member Author

!ci-build-me

@dkijania
Copy link
Member Author

!ci-build-me

@dkijania
Copy link
Member Author

!ci-build-me

@dkijania
Copy link
Member Author

!ci-build-me

@dkijania
Copy link
Member Author

!ci-build-me

@dkijania dkijania force-pushed the dkijania/mina-profile-docker branch 2 times, most recently from 1da28d1 to f58a2fa Compare September 26, 2023 08:15
@dkijania
Copy link
Member Author

!ci-build-me

@dkijania
Copy link
Member Author

!ci-build-me

1 similar comment
@dkijania
Copy link
Member Author

!ci-build-me

@dkijania dkijania force-pushed the dkijania/mina-profile-docker branch from ea811eb to f9b5281 Compare October 4, 2023 20:10
@dkijania
Copy link
Member Author

dkijania commented Oct 4, 2023

!ci-build-me

1 similar comment
@dkijania
Copy link
Member Author

dkijania commented Oct 6, 2023

!ci-build-me

@dkijania dkijania self-assigned this Oct 6, 2023
@dkijania
Copy link
Member Author

!ci-build-me

@dkijania dkijania force-pushed the dkijania/mina-profile-docker branch 2 times, most recently from 48f4591 to d58d24c Compare October 20, 2023 15:39
@dkijania dkijania force-pushed the dkijania/mina-profile-docker branch from d58d24c to aee05d8 Compare October 20, 2023 15:50
@dkijania
Copy link
Member Author

!ci-build-me

@dkijania
Copy link
Member Author

!ci-build-me

@dkijania
Copy link
Member Author

!ci-build-me

@dkijania
Copy link
Member Author

!ci-build-me

@dkijania
Copy link
Member Author

!ci-build-me

@deepthiskumar
Copy link
Member

!approved-for-mainnet

@deepthiskumar deepthiskumar merged commit 9b183f7 into berkeley Oct 24, 2023
1 check passed
@deepthiskumar deepthiskumar deleted the dkijania/mina-profile-docker branch October 24, 2023 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stevenplatt stevenplatt stevenplatt approved these changes

Assignees

@dkijania dkijania

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dkijania @deepthiskumar @stevenplatt