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

KP-9048 Fix reittidemo video not found #5

Merged
merged 1 commit into from
Oct 2, 2024
Merged

Conversation

aajarven
Copy link
Member

The video file was not found from the specified location on korp2, and the localhost request made from a user's machine has likely never worked anyway. Now the config matches the location of the actual file.

The video file was not found from the specified location on korp2, and
the localhost request made from a user's machine has likely never
worked. Now the config matches the location of the actual file.
@janiemi
Copy link

janiemi commented Sep 26, 2024

@aajarven, that should work as long as the video file is found at https://www.kielipankki.fi/media/reittidemo/reitti_a-siipeen.mp4.

I’m also wondering how the localhost can have worked in production Korp.

@mmatthiesencsc
Copy link
Collaborator

This looks good, and in a hurry I would have merged to main. But we agreed to be more flexible in testing, so leaving this PR and the branch open.

@janiemi
Copy link

janiemi commented Oct 1, 2024

@mmatthiesencsc, I think it would be nice to have this in the upcoming Korp3, so what is the advantage of not merging? This is such a simple fix that I don’t see any need for changes, unless the actual location of the file is changed.

Or would Korp3 (and the future korp-test) correspond to branch config/dev? Should I then actually have based #4 and #6 on dev and not master? (dev would have to be updated to master first.)

@mmatthiesencsc
Copy link
Collaborator

@janiemi This is a misunderstanding, we do want to have this in production. I just kept it "open" for Anni to be able to test a mechanism that makes it possible in the future to use any branch with Ansible. So it is a technicality. As to your "dev" suggestion above, if "dev" means staging, so very close to production, we should consider it, I think.

@aajarven
Copy link
Member Author

aajarven commented Oct 2, 2024

I have now verified that CSCfi/Kielipankki-korp-ansible#39 is able to install configuration from this branch, so I'm merging this. Thanks for the reviews!

For now, we are using master branches when provisioning korp3, so I'd say that the merged PRs are in the right place.

@aajarven aajarven merged commit 2876b05 into config/master Oct 2, 2024
1 check passed
@janiemi
Copy link

janiemi commented Oct 2, 2024

@mmatthiesencsc and @aajarven, thanks for explaining and sorry about the misunderstanding.

We could perhaps at some point discuss what kind of a branch policy would be desired, but it may be partly related to the reorganization of the Kielipankki Korp repositories.

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