Skip to content

Conversation

@cp-20
Copy link
Contributor

@cp-20 cp-20 commented Jul 13, 2025

なぜやるか

part of #1073

やったこと

デプロイタイプに Function App を追加した

やらなかったこと

  • フロントエンドでの設定画面
  • Function App のデプロイ部分

レビューしてほしい観点

  • 既存の機能を破壊しないか
    • 既存のコードもリファクタリング的にいじってるので、既存の機能が破壊されないかが重要
  • 書き方がプロジェクトに沿ってるか

@github-actions
Copy link
Contributor

Preview (prod backend + PR dashboard) → https://1077.ns-preview.trapti.tech/

@cp-20 cp-20 self-assigned this Jul 13, 2025
@cp-20 cp-20 marked this pull request as ready for review July 14, 2025 14:16
@cp-20 cp-20 requested a review from Copilot July 14, 2025 14:21
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Adds support for deploying Function Apps by introducing new function build configurations and extending existing build and deployment pipelines.

  • Introduces functionDest in builder state and new cleanup, extract, and save steps for function artifacts
  • Adds BuildConfigFunction* types across domain, repository converter, SQL migrations, and protobuf definitions
  • Refactors Dockerfile generation into generateBuildCmdDockerfile and updates pipeline steps in buildSteps

Reviewed Changes

Copilot reviewed 18 out of 19 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
pkg/usecase/builder/state.go Added functionDest field to track function artifact destination
pkg/usecase/builder/build_save_artifact.go Renamed and split artifact saving for static vs function builds
pkg/usecase/builder/build_registry.go Extracted registry cleanup into new method
pkg/usecase/builder/build_function.go Implemented function artifact extraction
pkg/usecase/builder/build_buildpack.go Added function buildpack support
pkg/usecase/builder/build_buildkit.go Centralized Dockerfile generation and added function cmd steps
pkg/usecase/builder/build.go Updated build pipeline with function build cases
pkg/infrastructure/repository/repoconvert/* Mapped new function build configs and deploy type
pkg/infrastructure/repository/models/boil_types.go Extended enums for function build and deploy types
pkg/domain/app_build_config.go Defined FunctionConfig and new build config types
pkg/domain/app_artifact.go Added constant for function artifact name
pkg/domain/app.go Added DeployTypeFunction
migrations/schema.sql Extended deploy_type and application_config enums
dashboard/.../applicationConfigSchema.ts Stubbed frontend schema for function build configs
api/proto/.../gateway.proto Added FunctionConfig and BuildConfigFunction* messages
Comments suppressed due to low confidence (2)

pkg/usecase/builder/build.go:139

  • New build steps for function buildpack are introduced here but no corresponding unit or integration tests have been added; consider adding tests to cover the function build path.
	case *domain.BuildConfigFunctionBuildpack:

pkg/usecase/builder/build_buildkit.go:227

  • A temporary Dockerfile is created but never cleaned up on error or after use; add defer cleanup() immediately after this line to remove the temp file.
	tmpName, cleanup, err := createTempFile("dockerfile-*", dockerfile)

Comment on lines 70 to 82
var artifactBytes bytes.Buffer
gzipWriter := gzip.NewWriter(&artifactBytes)
_, err = io.Copy(gzipWriter, file)
if err != nil {
return errors.Wrap(err, "copying file to gzip writer")
}
err = gzipWriter.Close()
if err != nil {
return errors.Wrap(err, "flushing gzip writer")
}

// Save artifact by requesting to controller
err = s.client.SaveArtifact(ctx, artifact, artifactBytes.Bytes())
Copy link

Copilot AI Jul 14, 2025

Choose a reason for hiding this comment

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

This buffers the entire artifact in memory before sending; for large artifacts it may be better to stream via an io.Pipe or chunked upload to reduce memory usage.

Suggested change
var artifactBytes bytes.Buffer
gzipWriter := gzip.NewWriter(&artifactBytes)
_, err = io.Copy(gzipWriter, file)
if err != nil {
return errors.Wrap(err, "copying file to gzip writer")
}
err = gzipWriter.Close()
if err != nil {
return errors.Wrap(err, "flushing gzip writer")
}
// Save artifact by requesting to controller
err = s.client.SaveArtifact(ctx, artifact, artifactBytes.Bytes())
pipeReader, pipeWriter := io.Pipe()
gzipWriter := gzip.NewWriter(pipeWriter)
// Start a goroutine to handle gzip compression and writing to the pipe
go func() {
defer pipeWriter.Close()
defer gzipWriter.Close()
_, err := io.Copy(gzipWriter, file)
if err != nil {
pipeWriter.CloseWithError(errors.Wrap(err, "copying file to gzip writer"))
}
}()
// Save artifact by requesting to controller
err = s.client.SaveArtifact(ctx, artifact, pipeReader)

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

これはあんまり問題にならなさそう?

return s.saveArtifact(ctx, st)
return s.saveTarGzArtifact(ctx, st)
}})
case *domain.BuildConfigFunctionBuildpack:
Copy link

Copilot AI Jul 14, 2025

Choose a reason for hiding this comment

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

[nitpick] The function build cases repeat common cleanup and save logic across multiple config types; refactoring those into a shared helper could reduce duplication and improve maintainability.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

一旦無視したい

@cp-20 cp-20 requested a review from pirosiki197 July 14, 2025 14:29
Copy link
Contributor

@pirosiki197 pirosiki197 left a comment

Choose a reason for hiding this comment

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

だいたいよさそうです!
既存のStaticBuildが壊れてないかだけ動作確認しておいてほしいです 🙏 (自分も確認します)

ch chan *buildkit.SolveStatus,
bc *domain.BuildConfigRuntimeCmd,
) error {
type BuildCmdOption struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

公開する必要はなさそう?

return nil
}

func (s *ServiceImpl) saveTarGzArtifact(ctx context.Context, st *state) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

saveStaticArtifactとかがよさそう


import "context"

func (s *ServiceImpl) buildRegistryCleanup(
Copy link
Contributor

Choose a reason for hiding this comment

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

cleanupRegistryとかがよさそう

}

func (s *ServiceImpl) buildStaticCleanup(
func (s *ServiceImpl) buildStaticExtract(
Copy link
Contributor

Choose a reason for hiding this comment

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

extractStaticArtifactとかがよさそう

)

func (s *ServiceImpl) buildStaticExtract(
func (s *ServiceImpl) buildExtractFolderToTar(
Copy link
Contributor

Choose a reason for hiding this comment

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

extractDirectoryとかがよさそう

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.

3 participants