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

docs: detail the usage of S3 cache policy #427

Merged
merged 6 commits into from
Dec 30, 2022
Merged

docs: detail the usage of S3 cache policy #427

merged 6 commits into from
Dec 30, 2022

Conversation

kayman-mk
Copy link
Collaborator

@kayman-mk kayman-mk commented Jan 9, 2022 •
edited
Loading

Description

As explained in #410 and #426 additional documentation is needed for

  • permissions needed to access the S3 cache. This is configured on the Runner instance and not on the Executor (as I thought too). But the documentation tolds a different story.

Migrations required

No

Verification

Just some comments. No verification done.

Documentation

We use pre-commit to update the Terraform inputs and outputs in the documentation via terraform-docs. Ensure you have installed those components.

npalm
npalm approved these changes Jan 13, 2022
View reviewed changes
Copy link
Collaborator

@npalm npalm left a comment

Choose a reason for hiding this comment

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

@kayman-mk looks good, but I assume you would like to got it merged after #410 Since the adpater is not supported yet

@kayman-mk
Copy link
Collaborator Author

@npalm Can be merged immediately I think as #410 is closed and works as documented by the Gitlab Runner team.

@npalm
Copy link
Collaborator

npalm commented Jan 18, 2022

@kayman-mk #410 is closed and not merged. Looks like the gitlab documentation is a bit misleading.

@npalm
Copy link
Collaborator

npalm commented Feb 27, 2022

@kayman-mk can we close this?

@kayman-mk
Copy link
Collaborator Author

@npalm Think it would be useful to have the comment regarding the instance profile for S3 bucket access in the code. The null_resource will be deleted soon. You may close this PR if you think we can go without it, yes.

@kayman-mk kayman-mk changed the title refactor: Add remarks to explain traps in the code refactor: explain the need of S3 policies Aug 6, 2022
@kayman-mk kayman-mk changed the title refactor: explain the need of S3 policies docs: explain the need of S3 policies Aug 6, 2022
@kayman-mk kayman-mk changed the title docs: explain the need of S3 policies docs: detail the usage of S3 cache policy Aug 6, 2022
@kayman-mk
Copy link
Collaborator Author

@npalm Merged the develop branch and removed half of the documentation as the code was deleted. There is only one remark left regarding the S3 policies. Merge or close? I don't care, but as there were problems in the past, I think it would be useful to have this remark in the code.

@npalm
Copy link
Collaborator

npalm commented Aug 6, 2022

On my list for Monday, thx for the work

@kayman-mk
Copy link
Collaborator Author

@npalm Branch updated

@kayman-mk kayman-mk merged commit 4baaf73 into cattle-ops:develop Dec 30, 2022
@kayman-mk
Copy link
Collaborator Author

Argh, the base branch was still develop and not main. Will create a new PR.

@kayman-mk kayman-mk mentioned this pull request Dec 30, 2022
Merged
npalm added a commit that referenced this pull request Dec 31, 2022
@npalm @kayman-mk
docs: detail the usage of S3 cache policy (#427) (#617)
b58cdc2 
* Add remarks to explain traps in the code

* Format code

* leave the comment, no code changes

Co-authored-by: Matthias Kay <[email protected]>
npalm pushed a commit that referenced this pull request Jan 1, 2023
@r33drichards @kayman-mk
fix: Invalid Function Arguement when passing bucket as arg (#266)
b9e73fe 
* fix Invalid Function Arguement when passing bucket as arg

see #265

* docs: detail the usage of S3 cache policy (#427)

* Add remarks to explain traps in the code

* Format code

* leave the comment, no code changes

Co-authored-by: Robert <[email protected]>
Co-authored-by: Matthias Kay <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@npalm npalm npalm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kayman-mk @npalm