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

Fix env tags on ADO #10902

Merged
merged 34 commits into from
Jun 21, 2024
Merged

Fix env tags on ADO #10902

merged 34 commits into from
Jun 21, 2024

Conversation

Sawthis
Copy link
Contributor

@Sawthis Sawthis commented Jun 4, 2024

Description

Changes proposed in this pull request:

  • Use base64 encoded string as value of Tags parameter in ado api call

  • Set EncodedTags ado api call parameter to true when using setting Tags parameter value

    The Tags parameter in ADO API call is a base64 encoded string. This allows passing special characters like quotes, dotes and slashes without need to escape them.
    Because the oci-image-builder pipeline must support encoded and raw Tags parameter value for the time of migration. An additional EncodedTags ADO API call parameter is set too true to indicate usage of base64. This will be removed once image-builder application will be switched to use encoded Tags parameter.

  • Added tag-base64 flag to accept tags values as base64 string

    The image-builder binary is used in oci-image-builder pipeline to parse tags and pass them to the build step. Because the Tags parameter can contain base64 encoded string, the flag must be used to pass it's value to the image-builder binary.

  • Use Build.BuildId env variable for determining the Azure DevOps CI System.

    This env variable is created by ADO and should be used as a standard way of detecting this CI system in our solutions.

  • Added dry-run mode with minimal functionality

  • Added debug mode and logger with structured logging.

    Added dry-run mode for testing purposes. This feature was already requested by user and it's implementation should be finished in image-builder github action dry-run #11025
    Debug mode and structured logging was added for testing purposes and should be used in whole image-buidler binary replacing existing usage of fmt package functions.

@Sawthis Sawthis requested review from neighbors-dev-bot and a team as code owners June 4, 2024 11:11
@kyma-bot kyma-bot added cla: yes Indicates the PR's author has signed the CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 4, 2024
@dekiel dekiel self-assigned this Jun 6, 2024
@Sawthis
Copy link
Contributor Author

Sawthis commented Jun 7, 2024

/retest

@dekiel dekiel added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 10, 2024
@kyma-bot kyma-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 20, 2024
@dekiel dekiel removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 21, 2024
decoded, err := base64.StdEncoding.DecodeString(o.tagsBase64)
if err != nil {
fmt.Printf("Failed to decode tags, error: %s", err)
os.Exit(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we return err instead of terminating application?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or at least match the previous handling with logger.Errorw
The code appears inconsistent, resembling a mix of different approaches where newer implementations are added without updating the older ones.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should return errors instead terminating application and we should use some logging lib instead fmt. But this should be done in whole application. I don't want to do this now, but started replacing in smaller parts when working on some changes. The changes you mention should be done in separate issue and cover whole application. I think such work we should do after removing support for local builds and Prow. The code base to change will be smaller and simpler.

@kyma-bot kyma-bot added the lgtm Looks good to me! label Jun 21, 2024
@kyma-bot kyma-bot merged commit 63f4f2b into kyma-project:main Jun 21, 2024
17 checks passed
dekiel added a commit to dekiel/test-infra that referenced this pull request Jun 21, 2024
* Replace all spaces for ADO Tags parameter

* Add support for AzureDevOps CI System

* Rename loadProwJobGitState to loadADOGitState

* Format double quotes for ADO tags parameter

* Use BUILD_BUILDID env variable

* Add comment for BUILD_BUILDID env variable

* Add comment for beautiful ReplaceAll command

* Send only values in Tags parameter

* Send only values in Tags parameter

* Revert "Send only values in Tags parameter"

This reverts commit c8bd817.

* Revert "Send only values in Tags parameter"

This reverts commit 4c6eb9e.

* Revert "Revert "Send only values in Tags parameter""

This reverts commit c1b516a.

* Revert "Revert "Send only values in Tags parameter""

This reverts commit 6804bea.

* Send only values in Tags parameter

* Use double quotes in the test

* Encode Tags to base64

* Add --parse-tags-only-base64

* Improve debug message

* Improve debug message

* Improve debug message

* Add additional tag-base64 flag

* Fix unit tests

* Replace VERSION with GOLANG_VERSION

* Support tags passed as comma separated base64 encoded string.

* Set fake github action output for dryRun mode.

* Fix expected test result.

* Encode Tags parameter in pipelines package instead in image-builder. Encoding should be done for each pipeline call not only from image-builder.

* Fix test expected value

* Set EncodedTags parameter to true when Tags parameter value is not empty.

* Add missing expected test value

* Remove unused function.

---------

Co-authored-by: dekiel <[email protected]>
@akiioto akiioto removed their assignment Jul 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Indicates the PR's author has signed the CLA. lgtm Looks good to me! size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants