Skip to content

feat: Compute project build parameters #82

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

Merged
merged 6 commits into from
Jan 29, 2022
Merged

feat: Compute project build parameters #82

merged 6 commits into from
Jan 29, 2022

Conversation

kylecarbs
Copy link
Member

@kylecarbs kylecarbs commented Jan 29, 2022

Adds a projectparameter package to compute build-time project
values for a provided scope.

This package will be used to return which variables are being
used for a build, and can visually indicate the hierarchy to
a user.

This modifies a prior migration which is typically forbidden,
but because we're pre-production deployment I felt grouping
would be helpful to future contributors.

This adds database functions that are required for the provisioner
daemon and job queue logic.
Adds a projectparameter package to compute build-time project
values for a provided scope.

This package will be used to return which variables are being
used for a build, and can visually indicate the hierarchy to
a user.
@kylecarbs kylecarbs self-assigned this Jan 29, 2022
@codecov
Copy link

codecov bot commented Jan 29, 2022

Codecov Report

Merging #82 (4117ee2) into main (b503c8b) will decrease coverage by 0.13%.
The diff coverage is 68.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #82      +/-   ##
==========================================
- Coverage   71.90%   71.76%   -0.14%     
==========================================
  Files          88       89       +1     
  Lines        3417     3563     +146     
  Branches       55       55              
==========================================
+ Hits         2457     2557     +100     
- Misses        751      786      +35     
- Partials      209      220      +11     
Flag Coverage Δ
unittest-go-macos-latest 67.42% <67.44%> (+0.07%) ⬆️
unittest-go-ubuntu-latest 69.76% <68.00%> (+0.27%) ⬆️
unittest-go-windows-latest 67.25% <67.44%> (+0.04%) ⬆️
unittest-js 78.30% <ø> (ø)
Impacted Files Coverage Δ
provisioner/terraform/provision.go 57.62% <50.00%> (-5.64%) ⬇️
coderd/projectparameter/projectparameter.go 67.44% <67.44%> (ø)
provisioner/terraform/parse.go 75.00% <100.00%> (+4.26%) ⬆️
peer/conn.go 76.82% <0.00%> (+0.91%) ⬆️

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 b503c8b...4117ee2. Read the comment docs.

@kylecarbs kylecarbs changed the base branch from main to parameterschema January 29, 2022 16:05
}

// Project parameters come third!
projectParameters, err := db.GetParameterValuesByScope(ctx, database.GetParameterValuesByScopeParams{
Copy link
Contributor

Choose a reason for hiding this comment

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

Following from the comment above - this projectParameters is a confusing name, because we use: for _, projectParameter := range projectHistoryParameters { above!

Some possible ideas:

  • We could rename projectParameters here to overriddenProjectParameters
  • We could rename projectParameter in those loops to projectHistoryParameter

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, I renamed em' all to projectHistoryParameter instead. Long name, but very clear!

Comment on lines 175 to 176
// Validates and computes the value for parameters; setting the value on "parameterByName".
func (c *compute) inject(scopedParameters []database.ParameterValue) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Good idea to factor this out to a helper method

Copy link
Contributor

@bryphe-coder bryphe-coder left a comment

Choose a reason for hiding this comment

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

Just some minor nits about comments and naming for the projectHistoryParameters / projectParameters when we compute the override - otherwise looks good to me!

Base automatically changed from parameterschema to main January 29, 2022 23:38
@kylecarbs kylecarbs merged commit b3c5bb3 into main Jan 29, 2022
@kylecarbs kylecarbs deleted the projectparameter branch January 29, 2022 23:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants